Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Ability to Write Boozmn.nc style output files #680
Add Ability to Write Boozmn.nc style output files #680
Changes from 48 commits
72da051
efb9245
a15b3f3
64b9b29
d20de7a
a11e1ab
279535b
7aa6318
1600f49
36749f6
84fd8f7
2aed44e
8a1f5b8
4a0014f
31614d8
46f1eda
5e69f19
40e24c8
b2d9ca4
b3b8e04
d3ec3eb
4f78956
e1860ac
d827c78
c05290f
0302761
31ca5d1
137c2eb
8bd6497
eaa7d00
daf4e7a
5d3585d
691203f
8b1930f
24166c2
08b46f4
3bb0683
e86448e
5bc1c76
cded97b
1d14e72
8938384
537b7ec
b9032b7
6d89589
d9646b3
f9e0619
4b0db03
96223bd
a972732
fa24b5a
6701d38
111b8d9
e08df50
75b65ec
e0804a5
fc68d35
4d48c7a
1fbf137
8b220bc
5a4dd8b
e0a9efb
5597e24
a147231
230b32a
0db93d4
92bf4fc
3e664cf
3d38ea7
9b01ab0
d47c658
46d597f
86b8cc0
0a687f9
c72360a
3f056df
aa06111
433d35c
152b5f1
cc1e827
bf97b5f
25d1864
b1147d4
64e5bc6
ee7e016
e0035a0
bf41545
c0dc846
c6de3fd
4a0da36
e53e29d
46efc0a
decbc52
f00189a
4fed74d
1e05a2a
6ab0cd4
8a3e784
033e6ec
f7fb3c6
2edea11
f263a26
1ab3d81
afb04fe
74a08d0
1590f2f
342a5e9
4d8f088
185315f
276ef14
2ce7d0a
f64ceaa
4e1a066
8a37f46
4e220c5
82b7231
8eccfd3
aa2de0d
f2b34ce
5196a0d
4345dc4
7d0a2bb
404b693
f579d34
385e9de
fcfa68d
fe0ffe4
ba487f4
d8e2c59
c1a5850
d7e6d39
0e48b04
b673533
84a4236
fe4c0f2
f2b7711
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be a compute function? If this normalization is specific to the boozmn output format, I think it should only be applied there. See the next comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is not specific to boozmn, this is the normalization used when you compute the boozer transform in DESC, I just moved it from the compute functions of
B_mn
to its own thing to avoid recomputationThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah OK I think this is needed because we aren't doing the usual
transform.fit()
and instead is more like an integration for each mode number.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be cleaner to not include this normalization factor here so that the resulting transform matrix is consistent with the native DESC double-Fourier basis convention. Other normalization choices are somewhat arbitrary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure but this is already used with the boozer nodes, and is only used in the boozer transform part. I include the norm multiplication here to avoid unnecessary computations as much as possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think the
"Boozer transform modes norm"
does not need to be its own compute quantity and could be moved into this function, because this is the only placed they are ever used. And the normalizations are somewhat meaningless on their own.