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

Getting surfaces of a box monitor #147

Merged
merged 11 commits into from
Jan 24, 2022
Merged

Getting surfaces of a box monitor #147

merged 11 commits into from
Jan 24, 2022

Conversation

shashwat-sh
Copy link
Contributor

Added FieldMonitor.surfaces() to monitor.py, and a corresponding test function test_monitor_surfaces_from_volume() in test_components.py.

For an object of class FieldMonitor associated with a box, the method FieldMonitor.surfaces() returns a list of 6 "surface" monitors, each one corresponding to the 6 sides of the box.

Would be useful to review that the functions are written in an efficient "pythonic" way - feedback on style etc. would be great!

@shashwat-sh shashwat-sh requested a review from tylerflex January 21, 2022 19:55
Copy link
Collaborator

@tylerflex tylerflex left a comment

Choose a reason for hiding this comment

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

Needs a few minor changes, but overall looks good!

@shashwat-sh shashwat-sh requested a review from tylerflex January 21, 2022 23:21
Copy link
Collaborator

@tylerflex tylerflex left a comment

Choose a reason for hiding this comment

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

Let's make one more change: I think it will be useful for the FieldTimeMonitor to also have this functionality. Can we move this method to AbstractFieldMonitor and generalize the stuff on line 197? For example, for each of the new surfaces, we can make a copy of self using self.copy(deep=True) and assign the correct centers, sizes, and names to the copies. Let me know if this makes sense.

@tylerflex
Copy link
Collaborator

Looks good. Only two comments for things to fix and I'll merge:

  • In the docstring, let's replace "box monitor" with "field monitor", since all monitors are technically box monitors and we dont use that terminology in other places.
  • Very minor, nitpicking comment: when we are appending the new monitors to the return list, let's initialze the new monitor first (mon_new = self.copy(deep=True)) and then modify it mon_new.size = ... before adding to the list. It's not a big deal, I just think this is a bit cleaner.

Thanks a lot!

@tylerflex tylerflex merged commit 7cd96c4 into develop Jan 24, 2022
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