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

Default Tier Message Creation #85

Open
KaraMelih opened this issue Dec 12, 2023 · 1 comment · May be fixed by #123
Open

Default Tier Message Creation #85

KaraMelih opened this issue Dec 12, 2023 · 1 comment · May be fixed by #123
Assignees

Comments

@KaraMelih
Copy link
Collaborator

KaraMelih commented Dec 12, 2023

The current implementation allows for creating all

SNEWSHeartbeatMessage, SNEWSTimingTierMessage, SNEWSSignificanceTierMessage, SNEWSCoincidenceTierMessage, SNEWSRetractionMessage

type messages. Under the snews_pt.messages class they are all defined as a child class of the SNEWSMessage and have their respective reqfields which is checked with has_required_fields() function upon creation.

However, these required fields are defined as follows;

class SNEWSCoincidenceTierMessage(SNEWSMessage):
    """Message for SNEWS 2.0 coincidence tier."""

    reqfields = [ 'neutrino_time' ]
    fields = SNEWSMessage.basefields + reqfields + [ 'machine_time', 'p_val', 'is_test' ]

    def __init__(self, neutrino_time=None, p_val=None, **kwargs):
        super().__init__(self.fields,
                         neutrino_time=self.clean_time_input(neutrino_time),
                         p_val=p_val,
                         **kwargs)

    def is_valid(self):
       ....

So that there are always defaults for the required fields. The validation ( self.is_valid() ) is checked only later when the message is submitted, and then it properly shows that the created message is not valid.

Do we want this behavior or should we also immediately check if the message has required fields and is valid?

There is a second danger with the SNEWSCoincidenceTierMessage where it creates a default message with the current time as the neutrino time. So by default, it has all the required fields (i.e. neutrino time) and it is valid, which might lead to problems.

EDIT: if the default value is None function has_required_fields() actually does complain. The issue arise from the clean_time_input() function which is invoked before the has_required_fields() and replaces None with -> datetime.now() so, it silently becomes a valid message.

@KaraMelih
Copy link
Collaborator Author

Similar problem also occurs in SNEWSTimingTierMessage class

class SNEWSTimingTierMessage(SNEWSMessage):
    """Message for SNEWS 2.0 timing tier."""

    reqfields = [ 'timing_series' ]
    fields = SNEWSMessage.basefields + reqfields + [ 'machine_time', 'p_val', 'is_test' ]

    def __init__(self, p_val=None, timing_series=None, **kwargs):
        super().__init__(self.fields,
                         p_val=p_val,
                         timing_series=[self.clean_time_input(t) for t in timing_series],
                         **kwargs)

by default the timing_series argument is None and __init__ function tries to iterate over None. If it is a list of none i.e. [None] then it would also create a message with the current time.

It might be better if the self.clean_time_input() function rejects, or at least complains about None's and tells the user "time was None appending the current datetime" otherwise it will silently change the value without raising any suspicion

@justinvasel justinvasel linked a pull request Aug 20, 2024 that will close this issue
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 a pull request may close this issue.

2 participants