Skip to content

Fix keyword files with list of lists #226

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

Merged
merged 2 commits into from
Feb 18, 2024
Merged

Fix keyword files with list of lists #226

merged 2 commits into from
Feb 18, 2024

Conversation

jkuhl-uni
Copy link
Collaborator

Hi,
this is the first version of an implementation to solve #225. This is using a list of lists.
It can easily be adapted to use dicts.

@s-kuberski
Copy link
Collaborator

Hi,
regarding your comment in #225: Does your change break backward compatibility or would you consider your changes to be bug fixes?

@jkuhl-uni
Copy link
Collaborator Author

With ll. 239, my changes do not break backwards compatibility.
The functionality is:
If we only have one replicum or want to read the same configs from multiple replica, we can simply hand over one list of configs. Otherwise, we are also able to hand over multiple lists, such that we can import different configs from different replica.

Copy link
Owner

@fjosw fjosw left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this @jkuhl-uni. I left two small comments.

if isinstance(files, list):
if isinstance(files[0], list):
files = files[i]

Copy link
Owner

@fjosw fjosw Jan 21, 2024

Choose a reason for hiding this comment

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

What happens if files is not of type list[list[int]]? Maybe we should add an explicit exception here or is there a type check somewhere else that I overlooked?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi, sorry it took some time, I put explicit type checks in both places where kwargs.get('files') is used and just put two tests.

@@ -309,7 +313,7 @@ def read_sfcf_multi(path, prefix, name_list, quarks_list=['.*'], corr_type_list=
w = specs[3]
w2 = specs[4]
if "files" in kwargs:
ls = kwargs.get("files")
Copy link
Owner

Choose a reason for hiding this comment

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

Was this a bug? Did the code work before when specifying the kwarg files?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

AFAIK this worked, but the thing is that the files-keyword didn't do anything in this case... With this, it would give you the option to (at least) say which ids you want, however, this is of course nto the same kind of control you have in the other cases, where you can actually say which idls you want.
For this to work, we would need to implement something similar to #185 for this read method. I coudl also take care of that, but I need some time for that I think.

@fjosw fjosw linked an issue Jan 21, 2024 that may be closed by this pull request
@fjosw fjosw merged commit b930fab into develop Feb 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Files keyword for multiple reps in read_sfcf
3 participants