Fixes for multimessenger analysis #252

haukekoehn · 2023-09-28T09:28:13Z

No description provided.

Bug fix data_dump instead of data-dump

When interpolation type is tf, we only want to get the _tf model when we can access the remote models. This hack works for now, but maybe we should think about a new system of naming conventions.

mcoughlin

I do not quite understand the reason for the changes in this PR yet.

mcoughlin · 2023-09-28T11:50:54Z

nmma/pbilby/parser/analysis.py

@@ -12,7 +12,7 @@ def create_nmma_analysis_parser(sampler="dynesty"):

    analysis_parser = argparse.ArgumentParser(prog="nmma_analysis", parents=[parser])
    analysis_parser.add_argument(
-        "data-dump",


This just means --data-dump is the argument and then the argument is args.data_dump already.

but the data-dump is a positional argument and therefore it gets passed as args.data-dump which causes trouble

No I think it turns into data_dump. Can @sahiljhawar confirm the behavior perhaps?

well i just checked again and when i change it back to data-dump it becomes args.data-dump at least in pbilby/analysis/main.

No, it does not gets changed to data_dump. It remains data-dump and it cannot be accessed as an attribute of args

I would suggest to change data_dump from position argument (data_dump) to a normal argument (--data-dump) and see if that works.

@haukekoehn @sahiljhawar Ah that is presumably the difference. Sorry, I didn't realize that it maintains its name if positional.

@haukekoehn args.data-dump is not a valid syntax. In case the value of data-dump needs to be extracted it should be done as such getattr(args, "data-dump")). Also, if positional arguments (such as data-dump) are being used CLI, then only the order matters and none of the these works: --data-dump, --data_dump, data_dump, data-dump.

yeah now i changed it to --data-dump

mcoughlin · 2023-09-28T11:51:15Z

nmma/em/model.py

@@ -291,7 +291,7 @@ def __init__(
                if filters is None:
                    self.filters = model_filters

-            if os.path.isfile(modelfile):
+            elif os.path.isfile(modelfile):


This if statement is meant to stand on its own. Maybe the local_only flag is not doing what you want?

No, i think the local_only flag is alright, i think. This was just a quick fix from my side, where I use the Bu2022Ye model.
I wanted to prevent it from jumping in to the else statement below, where it tries to read in the _mag_tf model, which doesn't exist for Bu2022Ye.

As i said this is rather hacky and now that i look at it again probably inappropriate. I think the issue is that for Bu2022Ye os.path.isfile(modelfile) is False, because modelfile actually does not point to Bu2022Ye_tf.pkl but rather to Bu2022Ye.pkl which does not exist. But I don't know how to fix this consistently for the entirety of the models out there.

@haukekoehn If _tf.pkl is the problem, maybe try stripping off the _tf from Bu2022Ye_tf.pkl and then use it with --local-only

i did it the other way around, where now _tf is added to the modelfile variable, when the interpolation type is tensorflow. Maybe we can chat tomorrow about this.

changed so it no longer is a positional argument.

mcoughlin · 2023-10-02T13:35:39Z

nmma/em/model.py

@@ -280,7 +280,7 @@ def __init__(
                self.svd_lbol_model = None
        elif self.interpolation_type == "tensorflow":
            import tensorflow as tf
-
+            modelfile = os.path.join(self.svd_path, f"{core_model_name}_tf.pkl")


@bfhealy Maybe we need a check here for the old style? For folks already with the stack downloaded?

mcoughlin

I am mostly ok with this as long as it works. Just unsure given the move away from the _tf pickle files.

sahiljhawar · 2023-10-02T14:01:12Z

@mcoughlin @bfhealy Maybe for now keep support for _tf and raise deprecation warning until the zenodo models are updated. And then release a minor version after this issue is patched?

mcoughlin · 2023-10-02T14:15:54Z

@sahiljhawar @bfhealy @haukekoehn I agree. Can we add an if else at the _tf check instead of just always looking for _tf?

bfhealy · 2023-10-02T14:20:54Z

@mcoughlin @sahiljhawar I agree with this as well. I can add a temporary check for <model>_tf.pkl if <model>.pkl can't be found. This may also require a temporary modification to get_model in models.py, which currently strips the _tf suffix off of the .pkl files it attempts to download.

bfhealy · 2023-10-02T14:55:17Z

@mcoughlin @sahiljhawar Just pushed a commit - the default modelfile remains <model>.pkl, but if that file is not found and --interpolation-type is tensorflow, the code will look for <model>_tf.pkl locally before downloading the same from Zenodo. New comments in model.py and models.py list what to remove once files on Zenodo are updated.

sahiljhawar · 2023-10-02T15:52:49Z

@mcoughlin Merge-ready?

mcoughlin · 2023-10-02T15:53:38Z

@sahiljhawar good for me!

haukekoehn added 2 commits September 27, 2023 14:57

datadump argument in parser

2f15459

Bug fix data_dump instead of data-dump

avoid loading model_mag_tf file

df28618

When interpolation type is tf, we only want to get the _tf model when we can access the remote models. This hack works for now, but maybe we should think about a new system of naming conventions.

sahiljhawar requested review from bfhealy and mcoughlin September 28, 2023 10:30

mcoughlin requested changes Sep 28, 2023

View reviewed changes

sahiljhawar mentioned this pull request Sep 28, 2023

Unpickling error Bu2022Ye.pkl (invalid load key "\x0a") #251

Closed

Small fixes for the full multimessenger inference. Data dump parser is

a43970b

changed so it no longer is a positional argument.

sahiljhawar requested a review from mcoughlin October 2, 2023 13:30

mcoughlin reviewed Oct 2, 2023

View reviewed changes

Add temporary check for <model>_tf.pkl file

b4e3c48

sahiljhawar merged commit 32bd04e into nuclear-multimessenger-astronomy:main Oct 2, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for multimessenger analysis #252

Fixes for multimessenger analysis #252

haukekoehn commented Sep 28, 2023

mcoughlin left a comment

mcoughlin Sep 28, 2023

haukekoehn Sep 28, 2023

mcoughlin Sep 28, 2023

haukekoehn Sep 28, 2023

sahiljhawar Sep 28, 2023

sahiljhawar Sep 28, 2023 •

edited

Loading

mcoughlin Sep 28, 2023

sahiljhawar Sep 28, 2023 •

edited

Loading

haukekoehn Oct 2, 2023

mcoughlin Sep 28, 2023

haukekoehn Sep 28, 2023

sahiljhawar Sep 28, 2023 •

edited

Loading

haukekoehn Oct 2, 2023

mcoughlin Oct 2, 2023

mcoughlin left a comment

sahiljhawar commented Oct 2, 2023

mcoughlin commented Oct 2, 2023

bfhealy commented Oct 2, 2023

bfhealy commented Oct 2, 2023

sahiljhawar commented Oct 2, 2023

mcoughlin commented Oct 2, 2023

Fixes for multimessenger analysis #252

Fixes for multimessenger analysis #252

Conversation

haukekoehn commented Sep 28, 2023

mcoughlin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahiljhawar Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahiljhawar Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahiljhawar Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcoughlin left a comment

Choose a reason for hiding this comment

sahiljhawar commented Oct 2, 2023

mcoughlin commented Oct 2, 2023

bfhealy commented Oct 2, 2023

bfhealy commented Oct 2, 2023

sahiljhawar commented Oct 2, 2023

mcoughlin commented Oct 2, 2023

sahiljhawar Sep 28, 2023 •

edited

Loading

sahiljhawar Sep 28, 2023 •

edited

Loading

sahiljhawar Sep 28, 2023 •

edited

Loading