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

Compatibility with lvk #41

Open
wants to merge 47 commits into
base: main
Choose a base branch
from
Open

Compatibility with lvk #41

wants to merge 47 commits into from

Conversation

G-Sommani
Copy link
Contributor

To be more compatible with LVK, I:

  • Changed the unit in the multiorder maps from deg-2 to sr-1;
  • The unit is stored now in TUNIT2 in the header;
  • Changed the column name from PROBABILITY DENSITY [deg-2] to [PROBDENSITY].

Moreover, in plot.py there were problems bacause the new matplotlib doesn't support anymore the attribute QuadContourSet.collections. This affects only create_plot when dozoom is set to True. I tried by using get_paths() to fix the problem, but I am not totally sure it is really doing what it should. At least in general it is not anymore giving error. However, is the option dozoom=True still used? It seems to me only redundant because there is already create_plot_zoomed. Could we get rid of the option dozoom=True?

Copy link
Contributor

@tianluyuan tianluyuan left a comment

Choose a reason for hiding this comment

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

I think it's fine to remove the old dozoom option in create_plot, unless there's a reason to preserve it for legacy purposes? Would it make sense to match units and labels for the regular, non-multiorder FITs file?

skyreader/utils/handle_map_data.py Show resolved Hide resolved
@G-Sommani
Copy link
Contributor Author

I think it's fine to remove the old dozoom option in create_plot, unless there's a reason to preserve it for legacy purposes? Would it make sense to match units and labels for the regular, non-multiorder FITs file?

I agree that if there is a reason to preserve it for legacy purposes we should keep it. But how can we verify if there is any reason for doing so?
Regarding matching units, @jessiethw, would switching the probability to a density of probability in the flattened probability maps create problems in the programs for the FRA? Would they need to change accordingly? What do you think in general about that?

@G-Sommani
Copy link
Contributor Author

I think it's fine to remove the old dozoom option in create_plot, unless there's a reason to preserve it for legacy purposes? Would it make sense to match units and labels for the regular, non-multiorder FITs file?

I agree that if there is a reason to preserve it for legacy purposes we should keep it. But how can we verify if there is any reason for doing so? Regarding matching units, @jessiethw, would switching the probability to a density of probability in the flattened probability maps create problems in the programs for the FRA? Would they need to change accordingly? What do you think in general about that?

I saw now the reference in issue 27. Makes sense to do everything with healpy functions. Maybe better in a dedicated pull request?

@jessiethw
Copy link

I think it's fine to remove the old dozoom option in create_plot, unless there's a reason to preserve it for legacy purposes? Would it make sense to match units and labels for the regular, non-multiorder FITs file?

I agree that if there is a reason to preserve it for legacy purposes we should keep it. But how can we verify if there is any reason for doing so? Regarding matching units, @jessiethw, would switching the probability to a density of probability in the flattened probability maps create problems in the programs for the FRA? Would they need to change accordingly? What do you think in general about that?

It would require a change so that we handle them in FRA as probability density rather than probability, but it should be a constant offset in each pixel in the case of a flattened map (corresponding to the pixel area) so it shouldn't be a large change there

@tianluyuan
Copy link
Contributor

I think it's fine to remove the old dozoom option in create_plot, unless there's a reason to preserve it for legacy purposes? Would it make sense to match units and labels for the regular, non-multiorder FITs file?

I agree that if there is a reason to preserve it for legacy purposes we should keep it. But how can we verify if there is any reason for doing so?

Maybe sending a notice one of the realtime slack channels and if no one complains that would suffice? Also, if the functionality is changed/broken due to a change in the dependency then it doesn't make sense to preserve.

@G-Sommani
Copy link
Contributor Author

I think it's fine to remove the old dozoom option in create_plot, unless there's a reason to preserve it for legacy purposes? Would it make sense to match units and labels for the regular, non-multiorder FITs file?

I agree that if there is a reason to preserve it for legacy purposes we should keep it. But how can we verify if there is any reason for doing so? Regarding matching units, @jessiethw, would switching the probability to a density of probability in the flattened probability maps create problems in the programs for the FRA? Would they need to change accordingly? What do you think in general about that?

It would require a change so that we handle them in FRA as probability density rather than probability, but it should be a constant offset in each pixel in the case of a flattened map (corresponding to the pixel area) so it shouldn't be a large change there

Thanks! I changed the units for the flattened map.

In the header, I also added the UTC date of the event, and I changed the name of the MJD and its comment, so that everything is as in the headers of LVK

@tianluyuan
Copy link
Contributor

tianluyuan commented Jan 15, 2025

Thanks @G-Sommani for sending the message on slack about the old zoomed version. Should we go ahead and remove that?

I remember now that LVK also releases flattened maps. Is that something that we would do as well, i.e. release both multi-order and standard healpix? If so, perhaps it would be worth checking whether LVK flattened maps are in probability density or probability per pixel, and match to that. I sort of remember @jessiethw mentioning that those were per pixel now. I'm not strongly tied to either one but just wanted to bring that up. Sorry for the added noise if we were to revert those changes!

Once that's resolved should we try to get this merged in?

@G-Sommani
Copy link
Contributor Author

Thanks @G-Sommani for sending the message on slack about the old zoomed version. Should we go ahead and remove that?

I remember now that LVK also releases flattened maps. Is that something that we would do as well, i.e. release both multi-order and standard healpix? If so, perhaps it would be worth checking whether LVK flattened maps are in probability density or probability per pixel, and match to that. I sort of remember @jessiethw mentioning that those were per pixel now. I'm not strongly tied to either one but just wanted to bring that up. Sorry for the added noise if we were to revert those changes!

Once that's resolved should we try to get this merged in?

I removed the option dozoom from create_plot(). In future it would be nice to have everything handled with healpy as suggested in issue 27, but I guess better to dedicate a pull request just for that.

Regarding the LVK flattened maps, if I am not wrong, the aim would be to publish only multiorder maps, in case @blaufuss can confirm.

@blaufuss
Copy link
Member

From my point of view, muilti-order maps make for much smaller objects with high resolution, and are pretty standard in use now adays thanks to LVK.

But, that said, have no objection if we wanted to post both to a website archive, but would prefer to include the multi-order map as our default product...

@jessiethw
Copy link

jessiethw commented Jan 17, 2025

Thanks @G-Sommani for sending the message on slack about the old zoomed version. Should we go ahead and remove that?

I remember now that LVK also releases flattened maps. Is that something that we would do as well, i.e. release both multi-order and standard healpix? If so, perhaps it would be worth checking whether LVK flattened maps are in probability density or probability per pixel, and match to that. I sort of remember @jessiethw mentioning that those were per pixel now. I'm not strongly tied to either one but just wanted to bring that up. Sorry for the added noise if we were to revert those changes!

sorry for the delayed response from me. Yes LVK does have both, but (same as Erik suggests for us) multiorder is the default for them. The flattened from LVK are per pixel, so we could stick with that for the flattened.

@G-Sommani
Copy link
Contributor Author

Thanks @G-Sommani for sending the message on slack about the old zoomed version. Should we go ahead and remove that?
I remember now that LVK also releases flattened maps. Is that something that we would do as well, i.e. release both multi-order and standard healpix? If so, perhaps it would be worth checking whether LVK flattened maps are in probability density or probability per pixel, and match to that. I sort of remember @jessiethw mentioning that those were per pixel now. I'm not strongly tied to either one but just wanted to bring that up. Sorry for the added noise if we were to revert those changes!

sorry for the delayed response from me. Yes LVK does have both, but (same as Erik suggests for us) multiorder is the default for them. The flattened from LVK are per pixel, so we could stick with that for the flattened.

Thanks, @jessie! So better to remove the probability density from the flat maps? For me it is the same, just let me know which version would be the most preferable

@@ -42,7 +41,7 @@
class SkyScanPlotter:
PLOT_SIZE_Y_IN: float = 3.85
PLOT_SIZE_X_IN: float = 6
PLOT_DPI_STANDARD = 150
PLOT_DPI_STANDARD = 300
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that for some small contours with the probability maps the small definition was a problem when creating the all-sky map. Just making the definition double seemed to fix the problem.

@G-Sommani
Copy link
Contributor Author

Flattened map is now again with probability per pixel. If this is OK, I would proceed with the merge (also considering the higher dpi for the all-sky map)

column_names = ["PROBDENSITY"]
column_units = ["sr-1"]
return equatorial_map, column_names, column_units
column_names = ["PROBABILITY"]

Choose a reason for hiding this comment

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

very minor comment: LVK just uses PROB as the column name for their flattened maps. also probably worth including units for flattened as well, they use pix-1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

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.

6 participants