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

Add SNEWSMessage class hierarchy. #81

Merged
merged 40 commits into from
Nov 10, 2023
Merged

Add SNEWSMessage class hierarchy. #81

merged 40 commits into from
Nov 10, 2023

Conversation

sybenzvi
Copy link
Contributor

@sybenzvi sybenzvi commented Jun 2, 2023

PR for a new branch to address issue #73.

@sybenzvi sybenzvi added the enhancement New feature or request label Jun 2, 2023
@sybenzvi sybenzvi marked this pull request as draft June 2, 2023 21:42
@sybenzvi sybenzvi linked an issue Jun 2, 2023 that may be closed by this pull request
@sybenzvi sybenzvi marked this pull request as ready for review June 23, 2023 12:30
@sybenzvi sybenzvi requested a review from KaraMelih June 23, 2023 12:31
@sybenzvi
Copy link
Contributor Author

@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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@sybenzvi sybenzvi merged commit 8d9c256 into main Nov 10, 2023
2 checks passed
@sybenzvi sybenzvi deleted the spt-message-classes branch November 10, 2023 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

improve object encapsulation in snews_pt
2 participants