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

Extract axes from SimulationBuilder #996

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from
Draft

Conversation

bonjourmauko
Copy link
Member

This is a work in progress based on my willingness to give a proper answer to #933 and #934.

I hoped to have a more advanced proposal for a request for comments, but axes are a hard one to tackle properly.

What do you think? Any feedback at this point @benjello @guillett?

Details below.

New features

  • Introduce Axis, AxisArray, and AxisExpander.
    • It clearly separates the data logic from the domain logic (setting up a collection of axes coherently vs actually expanding a simulation over a given collection of axes).

Improvements

  • Introduce proper separation of concerns between an axis, a multi-dimensional axes collection, and its expansion.
    • Allows for a better readability and testability, and future improvement of the expand axes feature.
    • Adds more unit test coverage, including some untested invariants (like the number of counts for a parallel set of axes).

Deprecations

  • Deprecate add_parallel_axis, add_perpendicular_axis, and expand_axes.
    • Those functionalities are now provided by AxisArray and AxisExpander.

@bonjourmauko bonjourmauko added kind:refactor Refactoring and code cleanup policy:rfc Request For Comments: chime in! labels Apr 11, 2021
Copy link
Member

@benjello benjello left a comment

Choose a reason for hiding this comment

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

I am not an expert but it looks cleaner than before therefore I have no objection.

@bonjourmauko
Copy link
Member Author

bonjourmauko commented Apr 13, 2021

Thanks @benjello, to be more clear what I would like is to know if the assumptions I'm doing are the right ones in terms of field expertise. For example:

  • Does it make sense that all axes perpendicular to themselves have the same amount of counts, or steps?
  • Does it make sense to have more than 2 dimensions? (a 4-dimension axes collection is unplottable, but maybe it makes sense for a use case of yours).
  • Etc.

Most of the intent is not yet implemented but outlined in the documentation.

@benjello
Copy link
Member

  • Does it make sense that all axes perpendicular to themselves have the same amount of counts, or steps?
  • No
  • Does it make sense to have more than 2 dimensions? (a 4-dimension axes collection is unplottable, but maybe it makes sense for a use case of yours).
  • Yes

@bonjourmauko
Copy link
Member Author

Thanks @benjello, this is not yet ready to be merged, I wanted to be sure I was shooting at the right direction.

  • Does it make sense that all axes perpendicular to themselves have the same amount of counts, or steps?
  • No

So we could have, for example, something like:

axes[[2,2], [3], [4]]

But no:

axes[[5,2], [3], [4]]

?

@bonjourmauko bonjourmauko changed the title [RFC] Extract axes from SimulationBuilder Extract axes from SimulationBuilder Oct 4, 2021
@bonjourmauko bonjourmauko marked this pull request as draft October 4, 2021 10:48
@bonjourmauko bonjourmauko added this to the Add support for weeks milestone Sep 17, 2024
@bonjourmauko bonjourmauko mentioned this pull request Sep 17, 2024
@bonjourmauko bonjourmauko mentioned this pull request Sep 25, 2024
32 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:refactor Refactoring and code cleanup policy:rfc Request For Comments: chime in!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants