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

Added profiles from CAKE #236

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open

Added profiles from CAKE #236

wants to merge 26 commits into from

Conversation

AreWeDreaming
Copy link
Collaborator

  • Ripped out other references to profiles needs to be reviewed carefully

- Ripped out other references to profiles needs to be reviewed carefully
@AreWeDreaming AreWeDreaming requested a review from orso82 March 8, 2023 06:53
"core_profiles.global_quantities.v_loop": {
"COCOSIO": 11,
"PYTHON": "core_profiles_global_quantities_data(ods, {pulse})"
},
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to rip this out to make it work. Putting it back in causes:
``core_profiles.profiles_1d.0.grid.rho_pol_norm has no data

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orso82 Do you understand why @AreWeDreaming would get this error, if this is not removed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe core_profiles_global_quantities_data looks at core_profiles.profiles_1d.0.grid.rho_pol_norm for some reason, and that mapping was not in place?

},
"core_profiles.profiles_1d.:.electrons.temperature": {
"TDI": "\\{PROFILES_tree}::TOP.PROFILES.ETEMPFIT",
"treename": "{PROFILES_tree}"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, but not because of errors but because of multiple sources for the same field. I don't know how to resolve this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orso82 How does one specify in the machine mappings when the same quantity is available via a variety of MDSplus trees? Maybe passing an argument to ods.open that gets passed to the underlying new core_profiles_profile_1d?

@AreWeDreaming AreWeDreaming self-assigned this Mar 8, 2023
@AreWeDreaming AreWeDreaming added WIP Work in progress help needed labels Mar 8, 2023
Copy link
Member

@smithsp smithsp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks promising.

"core_profiles.global_quantities.v_loop": {
"COCOSIO": 11,
"PYTHON": "core_profiles_global_quantities_data(ods, {pulse})"
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orso82 Do you understand why @AreWeDreaming would get this error, if this is not removed?

},
"core_profiles.profiles_1d.:.electrons.temperature": {
"TDI": "\\{PROFILES_tree}::TOP.PROFILES.ETEMPFIT",
"treename": "{PROFILES_tree}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orso82 How does one specify in the machine mappings when the same quantity is available via a variety of MDSplus trees? Maybe passing an argument to ods.open that gets passed to the underlying new core_profiles_profile_1d?

@orso82
Copy link
Member

orso82 commented Mar 8, 2023

See the "options" section in the "*.json" files to see how options can be passed to mapping functions. For example, you should be able to do:

"__options__": {
  "EFIT_tree": "EFIT01",
  "PROFILES_tree": "ZIPFIT01",
  "analysis_type": "CERQUICK",
  "default_tree": "D3D",
  "fast_ece": false,
  "nref": 0,
  "revision": "BLESSED"
 },
...
"core_profiles.profiles_1d.:.electrons.density": {
  "PYTHON": "core_profiles_profile_1d(ods, {pulse}, {PROFILES_tree})"
 }

which can then be passed with

with ods.open("d3d", 133221, PROFILES_tree="mytree")
    plot(ods['core_profiles.profiles_1d.0.electrons.density'])

@orso82
Copy link
Member

orso82 commented Mar 8, 2023

Should the ability to read ZIPFITS be retained? Are you sure there is nothing that depends on that?

@AreWeDreaming
Copy link
Collaborator Author

@orso82 Thanks for the example. I am not suggesting to removed anything, I was just confused about what to do and wanted to have an example for what I was struggling with when asking for help. I didn't realize that I need to move the zipfit mapping out of the .json file and into my new routine. I'll add an if-clause and the tree name as an argument.
The Omas viewer should be able to show whatever Omas can load from zip fits.

-  The use of coniditionals seems to cause problems for the .json
- Code could probably use some refactoring
@AreWeDreaming
Copy link
Collaborator Author

The last commit has several problems:

  1. The "PROFILES_tree" extra argument has to include \', e.g. "\'ZIPFIT01\'" in order to not loose its string nature. This is not the case for "EFIT_tree" which is a very similar extra argument.
  2. PROFILES_tree="\'OMFIT_PROFS\'"and PROFILES_tree="\'ZIPFIT01\'" do not provide the same fields. This seems to confuse the .json file that wants to have a fixed set of fields for each mapping function regardless of its input argument.
  3. I also need uncertainties. I learned from Sean that the TDI function is error_of but I am not sure how to do this for all fields without explicitly looping. I am not sure if explicitly looping over each field might cause separate MDS+ requests for each quantity. I guess I could just create a query like this:
query = {
            "electrons.density.data_error_upper": "error_of(\\TOP.n_e)",
            "electrons.temperature.data_error_upper": "error_of(\\TOP.T_e)",
            "ion[0].density.data_error_upper": "error_of(\\TOP.n_D)",
            "ion[0].temperature.data_error_upper": "error_of(\\TOP.T_i)",
            "ion[0].velocity.toroidal.data_error_upper": "error_of(\\TOP.V_tor_C)",
            "ion[0].density.data_error_upper": "error_of(\\TOP.n_C)"
        }

@orso82
Copy link
Member

orso82 commented Mar 9, 2023

  1. see how it's done for charge_exchange.channel for example
  2. I need to see the error, if there is an error
  3. Indeed you'll have to query the uncertainties separately and then assign them to the ODS

@AreWeDreaming
Copy link
Collaborator Author

Ok, so I managed to sort out all the problems I think. It seems that there were some minor errors that then became several weird problems. Now I can switch between ZIPFIT/OMFIT_PROFS without issues and the tree specification works without \".
I also added the uncertainties.
We'll have to revisit this when the fields for *_fit have data in MDS+. For now, I am happy with this.

@AreWeDreaming AreWeDreaming removed help needed WIP Work in progress labels Mar 9, 2023
- Removed GS error evolution plot
- Also added labels for ion
- Load missing equilibrium quantities via OMFIT
- Added functions for improved unit and scale handling in plots
- Added extra optional argument `omas_viewer` to equilibrium summary
    - It plots `p`, `q`, `j_tor`, and `chi^2` for diagnostics
    - Creates annotations for the `chi^2` plot with identifiers
    - Shows contraints for `j_tor` and `p` from `code.paramters`
- Moved functions needed by mappings to seprate file
  - Moved the machine mapping decorator to a utilities file
  - Moved MDS+ related routines into their own file
- Added extra argument to pass `update_mapping` as an argument
- Removed `compile` statement as it was causing trouble for the debugger
@github-actions
Copy link

Stale pull request message

@smithsp
Copy link
Member

smithsp commented Jul 21, 2023

@AreWeDreaming should remove the no-pr-activity label if this PR is still valid. If it needs further testing, let's work on it. The last automatic test failed.

@github-actions
Copy link

Stale pull request message

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.

4 participants