cherab / solps

Other
6 stars 5 forks source link

Loading simulation from MDSPlus or raw files will fail at obtaining Balmer-alpha wavelengths if OpenADAS repository is not installed in the default location #65

Closed vsnever closed 2 years ago

vsnever commented 2 years ago

When converting Balmer-alpha radiation density from photons/s/m3 to W/m3, the OpenADAS repository is used to obtain the wavelengths of Balmer-alpha spectral lines: https://github.com/cherab/solps/blob/31ec6b5db25be9f31c9e7a992845ccbbf58e6919/cherab/solps/formats/raw_simulation_files.py#L191-L197

https://github.com/cherab/solps/blob/31ec6b5db25be9f31c9e7a992845ccbbf58e6919/cherab/solps/formats/mdsplus.py#L182-L188

If the OpenADAS data path is not the default one, the code will fail to locate the wavelength files.

Instead of adding optional atomic_data argument to load_solps_from_raw_output() and load_mesh_from_mdsplus(), I propose to define a constant:

BALMER_ALPHA_WAVELENGTH = {hydrogen: 656.279, deuterium: 656.101, tritium: 656.041}

The value for hydrogen is hardcoded in populate() anyway and the other two wavelengths are calculated using the hydrogen one.

Does anyone have any objection to hardcoding these values here?

jacklovell commented 2 years ago

I'd prefer to avoid adding hard-coded values just to avoid getting them from a repository: we should aim for DRY where possible. Why not make atomic_data=None an optional argument to these routines, and if it's not supplied by the user then the atomic data will be loaded from the default OpenADAS repository location, i.e. using atomic_data = OpenADAS().

That way users can provide their own custom atomic data if necessary (either from ADAS for simply from a custom repository path), but existing users' scripts won't break.

Mateasek commented 2 years ago

I agree with @jacklovell here. Having it as an optional parameter is the standard solution adopted in cherab, atomic_data is passed already to many models and plasma. Hard coding it here would result in having atomic data specified outside of the atomic data object/repository and that is against the philosophy, at least against how I understood it. It could cause a lot of errors and lengthily debugging, especially for anyone who would not remember it was added here. If you want different atomic related data, you can create a new fully separate and custom repository with well defined content. This is I think what we should follow.

vsnever commented 2 years ago

Thanks for your comments. I've added optional atomic_data parameter to load_solps_from_raw_output() and load_mesh_from_mdsplus() in the PR #66.

vsnever commented 2 years ago

Fixed in PR #66.