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

move models to JSON files #200

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

move models to JSON files #200

wants to merge 2 commits into from

Conversation

jamestwebber
Copy link
Member

@jamestwebber jamestwebber commented Mar 28, 2023

This should wait until after #198 as it was branched off from there.

I was working on building a custom model for some analysis and decided to write a PR to do something I believe @kvg had mentioned before--moving the pre-configured models into separate json files rather than hard-coding them in the model_utils module.

There are some pros and cons here:

  • the file is a lot shorter 👍
  • but of course there are a bunch of new files 👎
  • it's easy to stick new models in that folder 👍
  • using code meant constants to avoid typos for some tags, can't do that 👎
  • json doesn't like to have comments in it 👎

Also not totally sold on the organization of where I stuck things...I didn't want to bury it too deeply, but it should be inside the package so that it's properly installed and accessible by importlib.resources.

@jamestwebber jamestwebber changed the title Jtw json models move models to JSON files Mar 29, 2023
@jamestwebber jamestwebber requested review from kvg and jonn-smith March 29, 2023 01:06
@jamestwebber
Copy link
Member Author

After some thought, tweaking this to flatten the json directory into just models and specifying array vs cdna in the JSON itself (which allows people to use one file for both, as in the tutorial).

@jamestwebber jamestwebber marked this pull request as ready for review April 6, 2023 18:58
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.

1 participant