juglab / cryoCARE_pip

PIP package of cryoCARE
BSD 3-Clause "New" or "Revised" License
25 stars 14 forks source link

Update README.md : #60

Closed TomCrey closed 5 months ago

TomCrey commented 5 months ago

In the predict_config.json explanatory file, in the "overwrite" line: False, a few " " around False were missing.

tibuch commented 5 months ago

Hi @TomCrey

Thank you for opening this PR. I just looked into the json documentation and figured out that it should be:

"overwrite": false

Could you test if false works for you?

Best wishes

tibuch commented 5 months ago

This also means that the predict_config.json file is wrong, where it is currently "overwrite": "False".

TomCrey commented 5 months ago

With us, when we use : "overwrite: false ; the program refuses to run, displaying the following series of errors:

(cryocare_11) tcrey@GRE070809:~/Data/CryoCARE_test$ cryoCARE_predict.py --conf predict_config.json
2024-04-02 09:56:14.369308: I tensorflow/stream_executor/platform/default/dso_loader.cc:49] Successfully opened dynamic library libcudart.so.11.0
Traceback (most recent call last):
  File "/home/tcrey/miniconda3/envs/cryocare_11/bin/cryoCARE_predict.py", line 175, in <module>
    main()
  File "/home/tcrey/miniconda3/envs/cryocare_11/bin/cryoCARE_predict.py", line 114, in main
    config = json.load(f)
  File "/home/tcrey/miniconda3/envs/cryocare_11/lib/python3.8/json/__init__.py", line 293, in load
    return loads(fp.read(),
  File "/home/tcrey/miniconda3/envs/cryocare_11/lib/python3.8/json/__init__.py", line 357, in loads
    return _default_decoder.decode(s)
  File "/home/tcrey/miniconda3/envs/cryocare_11/lib/python3.8/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/home/tcrey/miniconda3/envs/cryocare_11/lib/python3.8/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 7 column 16 (char 188)

but when we use : "overwrite": "false" ; everything works.

TomCrey commented 5 months ago

ok I've just read the documentation and it must be used as a boolean, so write "overwrite": false and not "overwrite": False as written in the README.

my PR would be to change this in the README

tibuch commented 5 months ago

And the predict_config.json should be updated accordingly.

I am trying to reproduce your error.

tibuch commented 5 months ago

Can you provide a copy of the config file which fails for you? I am not able to reproduce the issue with the following code:

import json
with open('predict_config.json', 'r') as f:
    config = json.load(f)
TomCrey commented 5 months ago

sure, here's the version of predict_config.json that doesn't work for us: { "path": "./output_model/model_test.tar.gz", "even": "rec_l11_ts_008_half1.rec", "odd": "rec_l11_ts_008_half2.rec", "n_tiles": [4,4,4], "output": "denoised.rec", "overwrite": False, "gpu_id": 0 }

tibuch commented 5 months ago

I see, but changing it to:

{ 
    "path": "./output_model/model_test.tar.gz", 
    "even": "rec_l11_ts_008_half1.rec", 
    "odd": "rec_l11_ts_008_half2.rec", 
    "n_tiles": [4,4,4], 
    "output": "denoised.rec", 
    "overwrite": false, 
    "gpu_id": 0 
}

works for you?

TomCrey commented 5 months ago

I've just seen on this link: "https://github.com/juglab/cryoCARE_pip/commit/a3eb51c3d2ab7c1bc52067315774dab6e4e86576" that the error had already been reported but that overwrite had therefore been declared as a string and not as a boolean... I think it's a boolean, isn't it?

TomCrey commented 5 months ago

I see, but changing it to:

{ 
    "path": "./output_model/model_test.tar.gz", 
    "even": "rec_l11_ts_008_half1.rec", 
    "odd": "rec_l11_ts_008_half2.rec", 
    "n_tiles": [4,4,4], 
    "output": "denoised.rec", 
    "overwrite": false, 
    "gpu_id": 0 
}

works for you?

yes I think

TomCrey commented 5 months ago

so you'd have to change both the README and this commit: "https://github.com/juglab/cryoCARE_pip/commit/a3eb51c3d2ab7c1bc52067315774dab6e4e86576"

tibuch commented 5 months ago

As far as I can tell it must be a boolean: https://github.com/juglab/cryoCARE_pip/blob/master/cryocare/scripts/cryoCARE_predict.py#L119

As far as I know

if "False": 
   do-someting

will always evaluate to true, because "any-text" is evaluated to true.

tibuch commented 5 months ago

I see, but changing it to:

{ 
    "path": "./output_model/model_test.tar.gz", 
    "even": "rec_l11_ts_008_half1.rec", 
    "odd": "rec_l11_ts_008_half2.rec", 
    "n_tiles": [4,4,4], 
    "output": "denoised.rec", 
    "overwrite": false, 
    "gpu_id": 0 
}

works for you?

yes I think

Can you update the README.md and prediction_config.json to reflect this change? As far as I can tell this must be the correct implementation.

Thank you and best wishes!

TomCrey commented 5 months ago

yes of course!

TomCrey commented 5 months ago

I think all is good!

tibuch commented 5 months ago

Thank you very much!