search

SWIFTSIM / velociraptor-python

Python tools for using velociraptor catalogues, with SWIFT integration
GNU Lesser General Public License v3.0
0 stars 8 forks source link

Global masking #68

Closed james-trayford closed 3 years ago

james-trayford commented 3 years ago

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 https://github.com/SWIFTSIM/pipeline/pull/25, https://github.com/SWIFTSIM/pipeline-configs/pull/130.

MatthieuSchaller commented 3 years ago

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

james-trayford commented 3 years ago

@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 (https://github.com/SWIFTSIM/pipeline-configs/pull/130) where the global mask is specified and registered, note you will also need the https://github.com/SWIFTSIM/pipeline/pull/25 PR changes for auto_plotter_global_mask to be a valid parameter in config.yml

MatthieuSchaller commented 3 years ago

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

JBorrow commented 3 years ago

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.

james-trayford commented 3 years ago

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 commented 3 years ago

Everything looks good! Just added one more comment. Apart from that, I think this branch is ready to be merged after https://github.com/SWIFTSIM/pipeline/pull/25 is rebasedto the current master