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

Global masking #68

Merged
merged 7 commits into from
Jul 23, 2021
Merged

Conversation

james-trayford
Copy link
Collaborator

@james-trayford james-trayford commented Jun 22, 2021

Apply a global mask to autoplotter plots. This can be specified in the pipeline config.yaml, and constructed in registration.py. Without specification, no mask is applied.

Changes straddle the pipeline and pipeline-configs repositories see also PRs SWIFTSIM/pipeline#25, SWIFTSIM/pipeline-configs#130.

velociraptor-plot Outdated Show resolved Hide resolved
velociraptor/catalogue/catalogue.py Outdated Show resolved Hide resolved
velociraptor/catalogue/registration.py Outdated Show resolved Hide resolved
@MatthieuSchaller
Copy link
Member

Do you have an example of how to use this in the auto-plotter?

@james-trayford
Copy link
Collaborator Author

@MatthieuSchaller Thanks for the review, this should work in conjunction with the other two pull requests, I just tagged the lines in the pipeline-config PR (SWIFTSIM/pipeline-configs#130) where the global mask is specified and registered, note you will also need the SWIFTSIM/pipeline#25 PR changes for auto_plotter_global_mask to be a valid parameter in config.yml

@MatthieuSchaller
Copy link
Member

I am happy with this to be merged. Does what it should.

@JBorrow
Copy link
Member

JBorrow commented Jul 3, 2021

Has this been tested in production yet? I'm concerned about the use of x_mask = self.global_mask and then the modification of x_mask.

Copy link
Collaborator

@EvgeniiChaikin EvgeniiChaikin left a comment

Choose a reason for hiding this comment

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

Hi @james-trayford,

I reviewed your changes. Everything looks good and works as expected. I added a couple of comments, mostly to improve the readability of the code.

You may also want to add the global mask tag here https://github.com/james-trayford/velociraptor-python/blob/master/velociraptor-plot#L152 in case someone decides to use auto plotter separately from swift-pipeline.

velociraptor/autoplotter/objects.py Show resolved Hide resolved
velociraptor/autoplotter/objects.py Outdated Show resolved Hide resolved
velociraptor/autoplotter/objects.py Show resolved Hide resolved
@james-trayford
Copy link
Collaborator Author

Has this been tested in production yet? I'm concerned about the use of x_mask = self.global_mask and then the modification of x_mask.

I think this works, I've replaced the modification of x_mask to avoid conclusion, but I keep x_mask as a temporary array to combine the global and plot-level structure masks.

@EvgeniiChaikin
Copy link
Collaborator

Everything looks good! Just added one more comment. Apart from that, I think this branch is ready to be merged after SWIFTSIM/pipeline#25 is rebased to the current master

@EvgeniiChaikin EvgeniiChaikin merged commit b168a6b into SWIFTSIM:master Jul 23, 2021
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.

5 participants