cherab / core

The core source repository for the Cherab project.
https://www.cherab.info
Other
44 stars 24 forks source link

OpenADAS DEFAULT_REPOSITORY_PATH from environment variable #416

Open Mateasek opened 1 year ago

Mateasek commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@vsnever the suggestion CHERAB_ATOMIC_DATA should be kept for the default path of Cherab's atomic data from the https://github.com/cherab/core/pull/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 commented 1 year ago

Anyway, I think CHERAB_OPENADAS_DATA is clearer than just CHERAB_OPENADAS.

Mateasek commented 1 year ago

@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 commented 1 year ago

I looked quickly into https://github.com/cherab/core/pull/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 commented 1 year ago

@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 commented 1 year ago

@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 https://github.com/cherab/core/pull/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.