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

refactor some of the enhancement SPI lgoic and provider interface #1393

Merged
merged 3 commits into from
Oct 15, 2024

Conversation

haileyajohnson
Copy link
Contributor

changed and cleaned up the logic of the EnhancementProvider a bit

  • each provider now needs to know it's own attribute name
  • the logic of whether to apply an enhancement is made by VariableDS
    This is meant to partition things such that it's a bit safer to execute runtime-loaded Enhancements.

I'm also looking into making a Vectorize Enhancement, as mentioned in this issue, since that's something we want at CDIP as well. If/when I get it working I'll add it as an example in the docs, similar to the lighting IOSP.

@haileyajohnson haileyajohnson marked this pull request as ready for review October 14, 2024 21:55
Copy link
Contributor

@tdrwenski tdrwenski left a comment

Choose a reason for hiding this comment

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

This looks great, thanks! Definitely makes more sense to move the enhancement name to each provider.

@@ -130,11 +130,17 @@ public enum Enhance {
* x<0 --> 0 x>0 --> 1
*/
ApplyClassifier,
Copy link
Contributor

Choose a reason for hiding this comment

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

should we remove ApplyClassifier, ApplyNormalizer, ApplyStandardizer and instead use ApplyRuntimeLoadedEnhancements for those classes since those are being runtime loaded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I kinda think so too, it seems like if anyone has those attributes in their catalogs they specifically would want them to be on, can't see a reason to leave that fine-level control in.

@tdrwenski tdrwenski merged commit c46f966 into Unidata:maint-5.x Oct 15, 2024
8 checks passed
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.

2 participants