Skip to content

Fix/merge idx #172

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 4 commits into from
Apr 28, 2023
Merged

Fix/merge idx #172

merged 4 commits into from
Apr 28, 2023

Conversation

s-kuberski
Copy link
Collaborator

This pr fixes two bugs:

  1. When merging observables that are defined on different regular sets of configurations (i.e., the idls being ranges), the old version of the code made an attempt to merge those smartly. However, the corresponding piece of code did not take into account all possible combinations, such that merging range(1, 1011, 2) and range(1, 1010, 1) led to range(1, 1011, 1), clearly adding a configuration 1010 that was not present before.

    The merge of idls is now always performed via sorted(set().union(*idl)) when the sets of configurations differ. Afterwards, an attempt is made to check whether the resulting union can be expressed as range. The resulting overhead of this check is much smaller than the total time that is needed to merge observables that are based on different sets of configurations. Using union certainly adds extra time compare to the previous (wrong) solution of merging, but I failed to find an analytical solution to either determine the range that describes the union of ranges or returns the list that describes the union in the most general cases. For the merge of matching idl, nothing has changed.

    A similar change has been applied to _intersection_idx to have the same, correct, definition of finding the union and intersection of a list of idls.

    Some tests that had been written to check correctness have been wrong themselves. I corrected them and added a few more.

  2. The second fix concerns an edge case where the solution of _calculate_drho for large time separations #157 did lead to an exception, since the list in _calculate_drho was missing an element.

@s-kuberski s-kuberski requested a review from fjosw as a code owner April 28, 2023 07:33
@fjosw
Copy link
Owner

fjosw commented Apr 28, 2023

Thanks, that looks good, I will have a closer look later.

One thing that I noticed is the use of the generic except Exception which could potentially also catch nonintended exceptions. Could you replace it with the exception type that is thrown if the elements are not identical? (I was not very careful with these things in the past but try to improve it along the way)

@s-kuberski
Copy link
Collaborator Author

Good point. I'll try to catch the relevant exceptions.

@fjosw
Copy link
Owner

fjosw commented Apr 28, 2023

A more cosmetic thing that I noticed is that we reuse these groupby patterns a few times. We could consider refactoring them.

@s-kuberski
Copy link
Collaborator Author

Is it possible that you introduced the try...except because np.ndarrays would otherwise break the workflow because they are not iterable? I did not find any other case where, given that we only pass lists of lists, ranges and arrays, an exception would be thrown. The more sane thing would then to introduce something like

idl = [np.nditer(o) if isinstance(o, np.ndarray) else o for o in idl]

and to remove the try altogether.

I will think how to refactor the pattern.

@fjosw
Copy link
Owner

fjosw commented Apr 28, 2023

That might be the case. The other thing I could imagine are empty lists for which you would get an IndexError but that should be excluded anyways.

@s-kuberski
Copy link
Collaborator Author

I guess we would want an exception to be thrown in the case where we pass an empty list ;)

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.

The CI fails because of wrong dependencies in the latest IPython release (ipython/ipython#14053). I will still merge this and hope that they resolve the issue soon.

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.

2 participants