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] Default config name is used when saving config from napari plugin #389

Open
quantumjot opened this issue Aug 9, 2023 · 8 comments

Comments

@quantumjot
Copy link
Owner

quantumjot commented Aug 9, 2023

When exporting the config from the napari plugin, the default config name is used, which means it cannot be loaded until it has been renamed.

167 if config_name in self.configs and not overwrite:
    168     _msg = (
    169         f"Config '{config_name}' already exists - "
    170         "config names must be unique."
    171     )
--> 172     raise ValueError(_msg)
        _msg = "Config 'cell' already exists - config names must be unique."
    174 self.configs[config_name] = config
    176 return config_name
@paddyroddy
Copy link
Collaborator

When you click "Save configuration" it asks you to select a name. What you mean is if you select the name cell.json then you can't then load that in? Considering we have cell and particle as default ones, what exactly would you want doing? We have to have a name for those. We intentionally raise that error here:

# TODO: Make the combobox editable so config names can be changed within the GUI
if config_name in self.configs and not overwrite:
_msg = (
f"Config '{config_name}' already exists - "
"config names must be unique."
)
raise ValueError(_msg)

@quantumjot
Copy link
Owner Author

I think it needs to be set in the actual config.json file too, rather than just the filename:

btrack/btrack/config.py

Lines 21 to 27 in 2ec7728

class TrackerConfig(BaseModel):
"""Configuration for `BayesianTracker`.
Parameters
----------
name : str
A name identifier for the model.

@paddyroddy
Copy link
Collaborator

Sorry, I still don't follow

@quantumjot
Copy link
Owner Author

The name attribute of the TrackerConfig needs to be updated to match the new filename, before exporting it. If it's left as cell or particle the user can't load the saved config as those already exist as default options the plugin.

@paddyroddy
Copy link
Collaborator

Okay I get you now

@p-j-smith
Copy link
Collaborator

just linking to a related issue (though not the same) #243

@p-j-smith
Copy link
Collaborator

it would probably be good to handle config name clashes in the plugin too - someone still might save cell.json, in which case they couldn’t load the config. Maybe a dialogue could popup asking whether the current config should be overwritten, or if the name of the loaded one should be modified. Or the name could be modified automatically

@paddyroddy
Copy link
Collaborator

Attempted but ran out of time #393

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants