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 cylcache to use the same variables as Cylinder. #38

Merged
merged 1 commit into from
Sep 16, 2023
Merged

Conversation

michael-petersen
Copy link
Member

This pull simply brings cylcache in line with variable names used in src/Cylinder.cc.

It does not make the variables in exputils/EmpCylSL.cc follow the same naming convention. There, lmax and nmax still retain the old sense. However, this is a deeper code change (IMO), so I have left it for now. The variable names are all internally consistent -- just not consistent up to the top (user-exposed) level.

@michael-petersen
Copy link
Member Author

Forgot to tag: this fixes issue #37.

@michael-petersen
Copy link
Member Author

@The9Cat , did the threads test work? If so, I'd suggest the change get committed here. As far as I'm concerned, this is otherwise complete.

@The9Cat
Copy link
Contributor

The9Cat commented Sep 16, 2023

It took me a few hours to convince myself that the two bases (with single and multiple threads) were the same. Actually, they are not quite the same, but they are isomorphic. That is, some of the eigenfunction spaces have sign flips and come in different orders in the two files; that is n=16 in the single threaded file might be n=17 in the multithreaded file. But they are the same up to sign flips and permutations. This won't matter to the end result as long as the same basis file is used.

Good grief.

@The9Cat
Copy link
Contributor

The9Cat commented Sep 16, 2023

I do have a question for you on the names of cylcache parameters. Specifically, do we want those parameters to match the keys in Cylinder in EXP and Cylindrical in pyEXP? They are not the same now. E.g. nmax vs NORDER.

@michael-petersen
Copy link
Member Author

The changes in the issue_37 branch do update to NMAX, etc. I did choose to keep capitals for consistency in the file, but that could change. I think all the variables match with Cylinder.cc now, up to the case.

@The9Cat
Copy link
Contributor

The9Cat commented Sep 16, 2023

Oh, sorry: I changed branches to do the git cherry-pick for the EOF stuff and got confused which branch was which. I suppose this means I checked the wrong version. But if only the parameter names changed, the check itself is still valid.

@The9Cat The9Cat merged commit 420caa7 into main Sep 16, 2023
8 checks passed
@The9Cat
Copy link
Contributor

The9Cat commented Sep 16, 2023

I will go ahead and merge this...

@The9Cat The9Cat deleted the issue_37 branch September 16, 2023 19:23
@michael-petersen
Copy link
Member Author

Change summary:

  • Variable changes (NMAXFID, LMAXFID, NMAX, NCYLODD)
  • Deleted NOUT variable to reduce confusion

Thanks for the merge!

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.

2 participants