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

Build wheels for Windows #47

Closed
lpsinger opened this issue Jul 5, 2023 · 13 comments · Fixed by #64
Closed

Build wheels for Windows #47

lpsinger opened this issue Jul 5, 2023 · 13 comments · Fixed by #64
Labels
help wanted Extra attention is needed

Comments

@lpsinger
Copy link
Contributor

lpsinger commented Jul 5, 2023

Add a job to our GitHub Actions pipeline to build binary wheels for Windows.

This is a little tricky due to some rough edges in Windows support in f2py and cibuildwheel. See:

@lpsinger lpsinger added the help wanted Extra attention is needed label Jul 5, 2023
@lpsinger lpsinger changed the title Build binary wheels for Windows Build wheels for Windows Jul 5, 2023
lpsinger added a commit to lpsinger/radbelt that referenced this issue Jul 5, 2023
The Windows wheels have never worked. See nasa#47.
lpsinger added a commit that referenced this issue Jul 5, 2023
The Windows wheels have never worked. See #47.
@kokroo
Copy link

kokroo commented Aug 11, 2023

What are we using F2PY for exactly, in this project? I see just 1 or maybe 2 files with minimal Fortran code. I could either rewrite it in C easily or use Numba to statically compile it. @lpsinger

@lpsinger
Copy link
Contributor Author

That's correct.

@kokroo
Copy link

kokroo commented Aug 11, 2023

That's correct.

Can I submit PRs to this repo? Also, can I rewrite the whole thing in C, or statically compile some Python code using Numba?
I could easily use Cython to embed the C code within the codebase and instantly have a cross-platform compiled program.

If I rewrite it in Python, Numba can easily speed it up but I highly doubt it will be faster than C. Is there any benchmark that I could use to test the performance?

@lpsinger
Copy link
Contributor Author

You could rewrite the whole thing in C or even Python, but I would not be qualified to review it because I am not a heliophysicist and I wouldn't be able to tell if your code is correct.

@kokroo
Copy link

kokroo commented Aug 11, 2023

@lpsinger Do we have some automated tests to check the input vs output? If we have a test suite, the C version should be fine. If we don't have a test suite, we should have one anyway to make sure newer versions aren't broken.

On another note, I am not a heliophysicist either :) , but we both can obviously check what the code does and compare it 1:1 using tests.

@VallesMarinerisExplorer

Anyone know where these functions come from? Can rewrite it in python and test if necessary but wondering if there is some documentation. Will see if there is some on CCMC.

def INITIZE():
pass

def FELDCOF(YEAR):
pass

def FELDG(LAT, LON, HEIGHT, DIMO):
pass

def SHELLG(LAT, LON, HEIGHT, DIMO, ICODE, BAB1):
pass

def FINDB0(value1, value2):
pass

def TRARA1(IHEAD, data, L, BB0, EE, FLUX):
pass

@kokroo
Copy link

kokroo commented Oct 18, 2023

Anyone know where these functions come from? Can rewrite it in python and test if necessary but wondering if there is some documentation. Will see if there is some on CCMC.

def INITIZE(): pass

def FELDCOF(YEAR): pass

def FELDG(LAT, LON, HEIGHT, DIMO): pass

def SHELLG(LAT, LON, HEIGHT, DIMO, ICODE, BAB1): pass

def FINDB0(value1, value2): pass

def TRARA1(IHEAD, data, L, BB0, EE, FLUX): pass

These are from the FORTRAN code in the repo. I think the wheel only has a compiled program and not the source code. Are you on Windows?

@lpsinger
Copy link
Contributor Author

lpsinger commented Oct 18, 2023

Folks, just to be clear: I do not recommend attempting to port this code operation-for-operation in pure Python. It's a very old model; the only reason I selected this model is that it is one of the few trapped particle flux models that actually has public source code available. Moreover, this implementation of the model has known numerical accuracy issues. What would be better is to take the time to understand the physics and come up with a more stable numerical implementation. I am an astronomer, not a heliophysicist, so unfortunately that is outside of my expertise to write or review.

Worse, this particular project is encumbered by the NASA Open Source Agreement (which is not an accepted open source license) and NASA's cumbersome contributor license agreement process, which limits my ability to accept third party contributions into it. This isn't true for all NASA projects, but unfortunately it is for this one. I requested to relicense it under a permissive, mainstream, open source license, but my employer said no.

I'm really sorry to be so discouraging about this. I am excited that you want to make this project better. But realistically, the best way for you to do that would be to fork it.

@kokroo
Copy link

kokroo commented Oct 19, 2023

@lpsinger Thanks for this insight. Regarding "NASA's cumbersome contributor license agreement process", I have already been through it for NASA's OnAIR project so I understand that it might be cumbersome for you to set that up.

Is it possible for you to replicate the contributions under your own name? If not, I would be more than glad to release a forked version which already works on macOS and Windows as well. In that case, I assume I would also have to create another package on PyPI. Could such a fork be linked to from this repo's description?

I am not even sure if NASA's license allows forks to be published.

@VallesMarinerisExplorer

Yes I am on windows. @lpsinger Do you have any links or specs on what the numerical inaccuracies are?

@lpsinger
Copy link
Contributor Author

@lpsinger Thanks for this insight. Regarding "NASA's cumbersome contributor license agreement process", I have already been through it for NASA's OnAIR project so I understand that it might be cumbersome for you to set that up.

I submitted the relicensing request a short time after you submitted your first PR. Oh well... 🤷

I am not even sure if NASA's license allows forks to be published.

You can modify and redistribute this code, and the license provides some guidelines on how to do that. Furthermore, the FORTRAN code from CCMC that I wrapped is not under NOSA (in fact, it is so old that it has no acknowledged license). So you could write your Python implementation directly from that.

@lpsinger
Copy link
Contributor Author

Do you have any links or specs on what the numerical inaccuracies are?

If you plot the trapped particle flux as a function of latitude and longitude, over a patch that includes the South Atlantic Anomaly, you'll see an obvious striped pattern that is not physical.

lpsinger added a commit to lpsinger/radbelt that referenced this issue Nov 13, 2023
lpsinger added a commit to lpsinger/radbelt that referenced this issue Nov 13, 2023
lpsinger added a commit to lpsinger/radbelt that referenced this issue Nov 13, 2023
lpsinger added a commit to lpsinger/radbelt that referenced this issue Nov 13, 2023
lpsinger added a commit to lpsinger/radbelt that referenced this issue Nov 13, 2023
@lpsinger
Copy link
Contributor Author

Do you have any links or specs on what the numerical inaccuracies are?

If you plot the trapped particle flux as a function of latitude and longitude, over a patch that includes the South Atlantic Anomaly, you'll see an obvious striped pattern that is not physical.

See attached example plots.
Lvalue-ripples
saa

lpsinger added a commit to lpsinger/radbelt that referenced this issue Nov 13, 2023
lpsinger added a commit that referenced this issue Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants