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

Configuration File for TermSet validations #1016

Merged
merged 85 commits into from
Mar 28, 2024
Merged

Configuration File for TermSet validations #1016

merged 85 commits into from
Mar 28, 2024

Conversation

mavaylon1
Copy link
Contributor

@mavaylon1 mavaylon1 commented Dec 11, 2023

Motivation

What was the reasoning behind this change? Please explain the changes briefly.

TLDR: Create a configuration file that sets which datatypes are associated with default NWB created termsets. This will be used for validation fields.

Note: This wrapping is done in the setter, requiring the field to be defined in the schema. If not, user the wrapper manually.
TODO:

  • Rename TermSetConfigurator to TypeConfigurator
  • Set the config file to use keys
  • use ruamel and not yaml
  • deep copy fix
  • doc tutorial

How to test the behavior?

Show how to reproduce the new behavior (can be a bug fix or a new feature)

Checklist

  • Did you update CHANGELOG.md with your changes?
  • Does the PR clearly describe the problem and the solution?
  • Have you reviewed our Contributing Guide?
  • Does the PR use "Fix #XXX" notation to tell GitHub to close the relevant issue numbered XXX when the PR is merged?

@mavaylon1
Copy link
Contributor Author

@oruebel Give a gander to the workflow above to see where my ideas are at.

Copy link

codecov bot commented Dec 11, 2023

Codecov Report

Attention: Patch coverage is 98.14815% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.65%. Comparing base (0000202) to head (0efd22b).
Report is 46 commits behind head on dev.

Files with missing lines Patch % Lines
src/hdmf/container.py 95.23% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1016      +/-   ##
==========================================
+ Coverage   88.55%   88.65%   +0.10%     
==========================================
  Files          45       45              
  Lines        9616     9717     +101     
  Branches     2738     2760      +22     
==========================================
+ Hits         8515     8615     +100     
  Misses        778      778              
- Partials      323      324       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

CHANGELOG.md Outdated Show resolved Hide resolved
requirements-opt.txt Outdated Show resolved Hide resolved
requirements-opt.txt Outdated Show resolved Hide resolved
src/hdmf/build/manager.py Outdated Show resolved Hide resolved
src/hdmf/common/__init__.py Outdated Show resolved Hide resolved
src/hdmf/container.py Outdated Show resolved Hide resolved
src/hdmf/container.py Outdated Show resolved Hide resolved
mavaylon1 and others added 4 commits March 27, 2024 21:21
Co-authored-by: Ryan Ly <[email protected]>
Co-authored-by: Ryan Ly <[email protected]>
Co-authored-by: Ryan Ly <[email protected]>
Co-authored-by: Ryan Ly <[email protected]>
src/hdmf/term_set.py Outdated Show resolved Hide resolved
src/hdmf/term_set.py Outdated Show resolved Hide resolved
@rly
Copy link
Contributor

rly commented Mar 28, 2024

@mavaylon1 At one of our recent meetings, we discussed not creating a global type config, but rather have a type config be associated with a particular type map, so that it is associated with whatever schema/namespaces are loaded at the moment. It looks like here, in the type map copy method, you pass the same type config around, so it is global instead. Can you change that?

@mavaylon1
Copy link
Contributor Author

mavaylon1 commented Mar 28, 2024

@mavaylon1 At one of our recent meetings, we discussed not creating a global type config, but rather have a type config be associated with a particular type map, so that it is associated with whatever schema/namespaces are loaded at the moment. It looks like here, in the type map copy method, you pass the same type config around, so it is global instead. Can you change that?

I must have misunderstood because I disagree. When we load a namespace, we are merging in the sense that the copy of the global type map can now navigate all namespaces. If a user wants to load a config that's unique to the namespace, they can call load and that too will be merged. That way I'm not searching for which config to use for which namespace. It's all under one.

My view is that the config file is associated with a namespace extension. Not the configurator.

Also if I get a copy of a the global type map. The type map that is used throughout hdmf, why would I have a config unique to that copy? That would imply I have to use that type map in other places. That's so much more difficult.

@mavaylon1 mavaylon1 enabled auto-merge (squash) March 28, 2024 17:05
@rly
Copy link
Contributor

rly commented Mar 28, 2024

Let's discuss whether the TypeConfigurator should be global or specific to an IO object in a separate PR.

@mavaylon1 mavaylon1 merged commit 244d17a into dev Mar 28, 2024
25 of 27 checks passed
@mavaylon1 mavaylon1 deleted the config branch March 28, 2024 17:09
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