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

OpenADAS DEFAULT_REPOSITORY_PATH from environment variable #416

Open
Mateasek opened this issue Jul 25, 2023 · 9 comments
Open

OpenADAS DEFAULT_REPOSITORY_PATH from environment variable #416

Mateasek opened this issue Jul 25, 2023 · 9 comments

Comments

@Mateasek
Copy link
Member

Hi,

I think it would be great if we could pull the default repository path for OpenADAS repository from environment variable (e.g. CHERAB_OPENADAS_DEFAULT_REPOSITORY_PATH).

The change I propose would be quite simple:

try:
    DEFAULT_REPOSITORY_PATH = os.environ["CHERAB_OPENADAS_DEFAULT_REPOSITORY_PATH"]
except KeyError:
    DEFAULT_REPOSITORY_PATH = os.path.expanduser('~/.cherab/openadas/repository')

This would allow simpler handling of system-wide installation of default repository and working with modules in linux without the need to change the code. Going back to the default path when the variable is not set keeps the backwards compatibility.

What do you think?

@jacklovell
Copy link
Member

I'm on board with this. We do a similar thing in cherab-mastu and cherab-jet with the CHERAB_CADMESH environment variable.

Two things:

  1. CHERAB_OPENADAS_DEFAULT_REPOSITORY_PATH is a bit of a mouthful. Perhaps just CHERAB_OPENADAS instead.
  2. This would need to be added to the documentation so people know how to use it.

@vsnever
Copy link
Member

vsnever commented Jul 25, 2023

Hi guys,

I support the idea of an environment variable for the atomic data repository path, but I wouldn't include OPENADAS in its name. In the future, the atomic data repository will include the data from multiple sources, not just OpenADAS. In fact, this transition is almost complete in #377. It is better to have a generic name initially so as not to change it in the future. I suggest CHERAB_ATOMIC_DATA.

@Mateasek
Copy link
Member Author

Thanks for suggestions, I should not be allowed to come up with naming, because I like my names too descriptive. What about CHERAB_OPENADAS_DATA?

@vsnever the suggestion CHERAB_ATOMIC_DATA should be kept for the default path of Cherab's atomic data from the #377 you pointed out, shouldn't it?

@vsnever
Copy link
Member

vsnever commented Jul 25, 2023

@vsnever the suggestion CHERAB_ATOMIC_DATA should be kept for the default path of Cherab's atomic data from the #377 you pointed out, shouldn't it?

Okay, but then after updating to a release that will have #377, the user will need to set a new environment variable, or we have to implement backward compatibility in #377.

@vsnever
Copy link
Member

vsnever commented Jul 25, 2023

Anyway, I think CHERAB_OPENADAS_DATA is clearer than just CHERAB_OPENADAS.

@Mateasek
Copy link
Member Author

@vsnever , I'm sorry I got confused between #364 and #377 ...

I looked quickly into #377 and why would it be problem to name the environment variable CHERAB_OPENADAS_DATA? I probably missed it, but there are no changes to the handling of the default repository path made, are there?

Getting back to both proposals, for openadas we can use the proposed environment variable CHERAB_OPENADAS_DATA and for the default data repository we can use CHERAB_DATA or am I missing something?

@vsnever
Copy link
Member

vsnever commented Jul 26, 2023

I looked quickly into #377 and why would it be problem to name the environment variable CHERAB_OPENADAS_DATA? I probably missed it, but there are no changes to the handling of the default repository path made, are there?

The #377 implements Alex's idea from #364 to solve this little mess we have in atomic data. It unifies the structure of the local atomic data repository and makes it independent of the data provider.

The #377 reduces the openadas submodule to just parsers and installers. The repository functions (add, get, update) and the atomic data interpolators go to cherab.atomic. So in cherab.atomic we have interpolators and repository functions for all atomic data specified the cherab.core.atomic.interface.AtomicData including the default atomic data and the data available in the full ADAS (Zeeman structure, fractional abundance). The default data is stored in the cherab/atomic/repository/default_data and copied to the user's atomic data repository when populate() is called (I probably need to add separate install_default methods for all the default atomic data we have). The user can replace the default data in their repository, just like any other data, using the add/update methods. The cherab.openadas.OpenADAS has been replaced with cherab.atomic.AtomicData, which now has an implementation for all methods specified in cherab.core.atomic.interface.AtomicData. Calling from cherab.openadas import OpenADAS will continue to work for backwards compatibility, but it will simply load the AtomicData class from cherab.atomic. Default user repository path changed to ~/.cherab/atomicdata/default_repository as it now includes data outside of OpenADAS.

The cherab-adas submodule will also be reduced to parsers and installers, I'm currently working on that. This will work like this, the user calls the installer from cherab.adas, which interrogates the ADAS routine and installs the data in the user's repository. After that, the data is available in cherab.atomic.AtomicData, so the separate cherab.adas.ADAS class is no longer needed.

I've already updated the documentation, so if you build the docs from #377 you'll see the new atomic data structure.

@Mateasek
Copy link
Member Author

@vsnever thanks for explanation. In this case I think using the CHERAB_DATA or CHERAB_DEFAULT_DATA is appropriate. Also, if you are already making such a big change in the #377, you might as well add the proposed functionality from this issue, if you don't mind. Or we can add it after it is accepted.

@vsnever
Copy link
Member

vsnever commented Jul 26, 2023

@vsnever thanks for explanation. In this case I think using the CHERAB_DATA or CHERAB_DEFAULT_DATA is appropriate. Also, if you are already making such a big change in the #377, you might as well add the proposed functionality from this issue, if you don't mind. Or we can add it after it is accepted.

Yes, I'll add this in #377, no problem. I still think CHERAB_ATOMIC_DATA is a better name because it's more specific. Who knows what other data source Cherab might use in the future. Also, CHERAB_DATA may refer to some user data, and I wouldn't be surprised if some users already use this name.

vsnever added a commit to vsnever/cherab-core that referenced this issue Aug 9, 2023
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

No branches or pull requests

3 participants