SNEWS2 / SNEWS_Publishing_Tools

Publishing Tool for SNEWS
BSD 3-Clause "New" or "Revised" License
2 stars 2 forks source link

Add SNEWSMessage class hierarchy. #81

Closed sybenzvi closed 9 months ago

sybenzvi commented 1 year ago

PR for a new branch to address issue #73.

sybenzvi commented 1 year ago

@KaraMelih , I've been sitting on this PR but I think it's ready for you to at least look at. The new messages.py module includes a main function that lets you quickly test the class instantiation. Just run

python snews_pt/messages.py

It will generate several JSON outputs and demonstrate construction of messages for the different tiers.

Notes:

  1. I have not touched any existing code, this is all new, so we could merge immediately if we wanted to.
  2. The functionality provided by the SNEWSTiersPublisher class in snews_pub.py is now largely replaced by SNEWSMessageBuilder in messages.py. I lied about not changing any client code; this class is renamed.
  3. One thing preventing this from dropping in as a complete replacement is the lack of a function that pushes out the messages, but that will be easy to add. First I think you should take a look and make sure there are no problems.
KaraMelih commented 1 year ago

Hi Segev, I hope you don't mind, I played around a little. \ The previous code was allowing user to display and interact with the message content before finally sending it to snews, I found it a bit non-intuitive with this version, so I added __repr__ methods (curiously, while the __repr_markdown__ works for the SNEWSMessage class, it did not work for the SNEWSMessageBuilder class. ) \

I also added a time check for each time input in the time series list.

I added the Publisher also directly in this module.

In the existing version, if you create two messages for two different tiers, the main arguments of one message is considered to be the meta for the other. To avoid that, before actually publishing, I look into the fields of all created messages, and append the other meta fields if they are not a part of any other tier.

Example, before doing this change:

SNEWSCoincidenceTierMessage
 ---------------------------
_id : LZ_CoincidenceTier_2023-06-26T15:29:47.598698
schema_version : 1.3.0
detector_name : LZ
neutrino_time : 2023-06-26T15:29:47.598698
 --------------------------- meta fields
p_values : [0.0007, 0.0008, 0.0009]
t_bin_width : 0.07
is_test : True

and


 SNEWSSignificanceTierMessage
 ----------------------------
_id : LZ_SignificanceTier_2023-06-26T15:29:47.598698
schema_version : 1.3.0
detector_name : LZ
p_values : [0.0007, 0.0008, 0.0009]
t_bin_width : 0.07
 ---------------------------- meta fields
neutrino_time : 2023-06-26T15:29:47.598698
p_val : 0.0007
is_test : True

Where obviously p_values and t_bin_width are appended as a meta field to first message because they are there for the SigTier message, and otherway around for the CoincTier. With the change, it just appends is_test field as it is not a part of any generated message's required fields.

KaraMelih commented 1 year ago

Still need to test the JSON interactions. I only tested using a jupyter notebook.
I am also not sure whether we should test the input types and validity in this module. e.g. if I pass

msg = messages.SNEWSMessageBuilder(test=1)

this still creates a message, and fails at a later stage when you try to send it. More dangerous is;

msg = messages.SNEWSMessageBuilder(detector_name=9999)

is also valid, and it sends it to snews without problem. So I think at the least the detector name should also be validated

sybenzvi commented 1 year ago

Thanks for checking, obviously both checks should be implemented. Could be done by the message builder or the message classes if we don't want invalid detectors to even be built into a message.

sybenzvi commented 10 months ago

@KaraMelih , I merged in your changes but didn't notice that with the new requirement of numpy >= 1.22.3 we can no longer use Python 3.7. Since Python 3.7 has reached end-of-life I think we may want to remove support for it.

sybenzvi commented 9 months ago

This is ready to merge but we need snews_cs to properly recognize the meta field altered in this PR. We will hold off the merger until that's complete.

sybenzvi commented 9 months ago

@KaraMelih confirms that snews_cs has a fix for the meta issue and we can merge this PR.