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

bug when add_deepimagej_config=True in build_model #368

Open
esgomezm opened this issue Dec 12, 2023 · 5 comments
Open

bug when add_deepimagej_config=True in build_model #368

esgomezm opened this issue Dec 12, 2023 · 5 comments

Comments

@esgomezm
Copy link
Contributor

Halo!

When building a model and adding the deepImageJ config with the IJ macros, if the pre and post-processing need to use the same pipeline (e.g., scale range), there is the issue of downloading just one macro rather than two different versions of it (pre+postprocessing).

For example, the following code will download the scale_linear.ijmfile twice (one for the preprocessing and one for the postprocessing) but with the same name and in the deepImageJ config, both pre and post-processing will point to the same ij macro file scale_linear.ijm but with the wrong parameters. Could it be possible to rename the downloaded macros accordingly to something like pre_scale_linear.ijm and post_scale_linear.ijm and include as such in the deepImageJ config?

# Preprocessing steps
bmz_preprpocess = [[{"name": "scale_range", "kwargs": {"min_percentile": 2.,
                                                       "max_percentile": 99.9,
                                                       "mode": "per_sample",
                                                       "axes": "xy"}},
                    {"name": "scale_linear", "kwargs": {"gain": 2,
                                                        "offset": -1,
                                                        "axes": "xy"}}]]
# Post processing steps
bmz_postprocess = [[{"name": "scale_linear", "kwargs": {"gain": 0.5,
                                                        "offset": 0.5,
                                                        "axes": "xy"}}]]

# Input & output specs
# ---------------------------------------
kwargs = dict(
  input_names=["input"],
  input_axes=["bcyx"],
  pixel_sizes=[pixel_size],
  preprocessing = bmz_preprpocess)

output_spec = dict(
  output_names=["output"],
  output_axes=["bcyx"],
  postprocessing=bmz_postprocess, 
  output_reference=["input"],
  output_scale=[4*[1]], 
  output_offset=[4*[0]]
)
kwargs.update(output_spec)
 
...

  build_model( ..., 
      add_deepimagej_config=True,
      **kwargs)

Thank you!

@FynnBe
Copy link
Member

FynnBe commented Dec 14, 2023

Implementing that would be possible (probably rather as part of the save_bioimageio_package and its related functions).
But allow me to raise the question: When will the DeepImageJ plugin be able to create the required macros on their own from the model description?
Including them explicitly in the package was always meant as a temporary solution...

@esgomezm
Copy link
Contributor Author

Hi hi

Implementing that would be possible (probably rather as part of the save_bioimageio_package and its related functions).

I can give it a try if you point me where this is done. I needed it for some zerocost notebook conversion that I just finished.

But allow me to raise the question: When will the DeepImageJ plugin be able to create the required macros on their own from the model description?
Including them explicitly in the package was always meant as a temporary solution...

Yes, you're right. JDLL has the functions implemented. I raised it to @arrmunoz already. Tagging @cfusterbarcelo also to keep track of this.

@FynnBe
Copy link
Member

FynnBe commented Dec 15, 2023

If only a few models are affected I suggest to fix the exported models manually.
If the next spec library release implements this as well, we can use f"{tensor_id}_{macro_file_name}" as final file name to avoid file name collisions. I have a draft for that already, but ideally we won't need a special deepimagej section under config soon and implement the required logic in DeepImageJ instead.

@esgomezm
Copy link
Contributor Author

Hi Fynn,

Thank you! As long as JDLL is updated with the specs, you are completely right and that's how it should be. So, do you want me to test the changes in the PR?

@FynnBe
Copy link
Member

FynnBe commented Jan 11, 2024

well, the above PR isn't ready to be merged, see bioimage-io/spec-bioimage-io#546 (comment)

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

No branches or pull requests

2 participants