nasa / radbelt

radbelt: An Astropy-friendly wrapper for the AE-8/AP-8 Van Allen belt model
Other
28 stars 9 forks source link

Build wheels for Windows #47

Closed lpsinger closed 7 months ago

lpsinger commented 12 months ago

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:

kokroo commented 10 months ago

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 commented 10 months ago

That's correct.

kokroo commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

@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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

@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 commented 8 months ago

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

lpsinger commented 8 months ago

@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... :shrug:

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 commented 8 months ago

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 commented 7 months ago

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