Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow multiple sources for the dataset #2276

Merged
merged 4 commits into from
Sep 17, 2024

Conversation

kif
Copy link
Member

@kif kif commented Sep 17, 2024

close #2275

@kif kif added the ready to merge Please review label Sep 17, 2024

_dataset_path = dataset_path[:-c]
path, base = posixpath.split(_dataset_path)
self._dataset_paths = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could go with the modifs l.155 to keep the relevant code together.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can make sense ... my idea was to keep the context manager part as small as possible, i.e. put this outside the context manager. No problem for moving it into the with hdf5file section.

Comment on lines 123 to 128
c=0
for i in dataset_path[-1::-1]:
if i.isdigit():
c+=1
else:
break
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can assume more on the dataset path?

Do something like i = dataset_path.find("images_"); _dataset_path = dataset_path[i+6:];

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer assume as little as reasonably possible. Why enforce the basename if not needed ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, we are still enforcing a particular format of basename here (it must have digits at the end and be contained in a group). So I thought that putting stronger conditions would allow the parsing to be more explicit.

Copy link
Collaborator

@EdgarGF93 EdgarGF93 Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would suppose changing the initData attributes again, but if we pass group_path and dataset_name on the initData method, with dataset_name None as default, and in this case, just iterates to get every dataset inside the group. That would be a general solution.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loic, it just imposes all datasets have the same prefix. If not, the sortoperation will not work as expected.
It is really a pity there is no way to enforce the order of dataset/groups in an HDF5 file. Those are just work around for this weakness.

Edgar, I am not in favour of changing the signature of methods which are supposed to be in the public API, even though I have no clue if anybody else is using it. As Linus said in https://lkml.org/lkml/2012/12/23/75 ... don't break users' programs. I like Linus because I feel really moderated in comparison with him :)

src/pyFAI/gui/pilx/MainWindow.py Outdated Show resolved Hide resolved
src/pyFAI/gui/pilx/MainWindow.py Outdated Show resolved Hide resolved
status_bar = self.statusBar()
if status_bar:
status_bar.showMessage(error_msg)
else:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is really worth continuing the execution of the function if the image group cannot be accessed ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are plenty of self.blabla initialized later on. Putting a return there would prevent them from being initialized. I have no clue what would be the behaviour of the application. You are the author more than me.
If you believe it is safe to return there, I see no objection.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave it like that, I'll check later.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes when I was writing and testing the widget, I moved the file or deleted the raw data, so there was no 2D pattern displayed but I still could test the map and the integrated pattern. I think the program should continue the execution, at least if we cannot see the data, we know immediately that there is a problem with the path of the data

kif and others added 3 commits September 17, 2024 15:10
Co-authored-by: Loïc Huder <42204205+loichuder@users.noreply.github.com>
Co-authored-by: Loïc Huder <42204205+loichuder@users.noreply.github.com>
src/pyFAI/gui/pilx/MainWindow.py Show resolved Hide resolved
status_bar = self.statusBar()
if status_bar:
status_bar.showMessage(error_msg)
else:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave it like that, I'll check later.

kif added a commit to kif/pyFAI that referenced this pull request Sep 17, 2024
@kif kif mentioned this pull request Sep 17, 2024
@kif kif merged commit 423aac7 into silx-kit:main Sep 17, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Please review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pilx] reads only the first HDF5 source dataset
3 participants