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

Added default check to register #271

Merged
merged 11 commits into from Jun 15, 2020
Merged

Conversation

WenzDaniel
Copy link
Collaborator

Checking if defaults are set consistently when registering a new plugin.

@JoranAngevaare
Copy link
Member

JoranAngevaare commented Jun 9, 2020

Hi Daniel, you beat me. I added:

if hasattr(plugin_class, 'takes_config'):
    for p in self._plugin_class_registry.values():
        for k ,v in p.takes_config.items():                
            if (k in plugin_class.takes_config and
                v.get_default('0') != plugin_class.takes_config[k].get_default('0')):
                raise ValueError(f'The value for option "{k}" for "{plugin_class.__name__}" is different from "{p.__name__}"')    

Some ideas:

  • Why would you expect to see plugins multiple times? Even if you would, you make a new list each time you call register so I'm not sure if it'd help.
  • I haven't ran into strax.InvalidConfiguration, could it be because I used the k in plugin_class.takes_config?
  • I think '0' is a safe default?
  • May I suggest for provides, plugin in self._plugin_class_registry.items(): ->for plugin in self._plugin_class_registry.values(): as you are not really using provides.

Other than that it looks good, I think we had a similar idea!

I'll add my comments as a review as good practice.

Copy link
Member

@JoranAngevaare JoranAngevaare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hereby the ideas expressed in the comment above.

strax/context.py Show resolved Hide resolved
strax/context.py Show resolved Hide resolved
strax/context.py Outdated Show resolved Hide resolved
strax/context.py Outdated Show resolved Hide resolved
@JoranAngevaare
Copy link
Member

JoranAngevaare commented Jun 9, 2020

By the way I've checked your implementation* and it works like a charm!

*See (at bottom of):
home/angevaare/software/straxferno/4.Contexts_run_selection_and_configuration.ipynb

ValueError: Two plugins have a different default value for the same option. The option "trigger_min_area" in "Events" takes as a default "100" while in "DummyClass" the default value is set to "110". Please change one of the defaults.

strax/context.py Outdated Show resolved Hide resolved
@WenzDaniel
Copy link
Collaborator Author

Thanks for your comments. As discussed, I added all needed changes.

@JoranAngevaare
Copy link
Member

Great! I think you only missed this one:

for plugin in self._plugin_class_registry.values():

@WenzDaniel
Copy link
Collaborator Author

WenzDaniel commented Jun 11, 2020

Great! I think you only missed this one:

for plugin in self._plugin_class_registry.values():

Super strange, I changed .items() into values(), but forgot to remove the provides field. Sometimes I doubt my mental health... :D
Good catch thanks.

Copy link
Member

@JelleAalbers JelleAalbers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Daniel and Joran!

@JelleAalbers JelleAalbers merged commit 6e2a363 into AxFoundation:master Jun 15, 2020
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 this pull request may close these issues.

None yet

3 participants