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

Allow variable-length inputs and outputs #23

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

AntoineBrunet
Copy link
Contributor

@AntoineBrunet AntoineBrunet commented Aug 27, 2021

This PR changes most subroutines inputs and outputs arrays to variable-length array, when possible.
In practice, it means that when ntime_max was the last dimension of a parameter array, I replaced it with ntime (if it was a parameter of the routine). This means that:

  • Most routines will now accept arbitrary-length inputs, not limited to ntime_max.
  • Most routines can be called with arrays as small as the actual number of requested points.

As a side effect, it fixes bug #21 which has been reported a few times. When ntime_max was not the last dimension of the array, such modification would break compatibility, so I did not make the changes. It happens in the following parts:

  • AE8_AP8.f
  • onera_desp_lib.f in the make_lstar_shell_splitting routine due to the alpha dimension (here we could change the constant Nalp to Nipa).
  • similarly, in LandI2Lstar.f, for the shell-splitting version of LAndI2Lstar.
  • sgp4_ele.f
  • AFRL_CRRES_models.f

We should probably think about how to appropriately document these changes in the API. Again, these modification should be compatible with legacy code (I checked with the IDL and python programs in the repository, but of course it does not cover every use case yet).

  • Pull request has descriptive title
  • Pull request gives overview of changes
  • New code has inline comments where necessary
  • Appropriate documentation has been written
  • Relevant issues are linked to (e.g. See issue # or Closes #)

@drsteve
Copy link
Contributor

drsteve commented Apr 14, 2022

I know this has been here a while, but it's on my list to take a closer look. Superficially it looks fine, but I'm trying to figure out whether the changes have side-effects...
My main aim is to make sure that there aren't any unforeseen issues when compiling for SpacePy (because we don't use the Makefile) and when calling stuff through the SpacePy wrapper.

Compilation seems fine. Maybe even fewer warnings than before?
We do get some errors showing up in our tests when I integrate these changes though, and I haven't tracked down the origin yet.

E.g.,

multiprocessing.pool.RemoteTraceback:
"""
ValueError: 0-th dimension must be 5 but got 0 (not defined).


The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/smorley/miniconda3/envs/default/lib/python3.9/multiprocessing/pool.py", line 125, in worker
    result = (True, func(*args, **kwds))
  File "/home/smorley/miniconda3/envs/default/lib/python3.9/multiprocessing/pool.py", line 48, in mapstar
    return list(map(*args))
  File "/home/smorley/github/spacepy/build/lib/spacepy/irbempy/irbempy.py", line 1739, in _multi_get_Lstar
    DALL = _get_Lstar(ticks, loci, alpha, extMag, options, omnivals, landi2lstar)
  File "/home/smorley/github/spacepy/build/lib/spacepy/irbempy/irbempy.py", line 1448, in _get_Lstar
    lm, lstar, bmirr, bmin, xj, mlt = func(*args)
ValueError: failed in converting 2nd argument `options' of irbempylib.make_lstar1 to C/Fortran array
"""

That could mean (probably means?) we need to update the wrapping, or it could point to a different issue. I'll take a look and see what I can find.
The other thing I want to look at is that there are a lot of things like implied DO loops that f2py can't parse (and complains about), so I'd be interested in making a pass over the code to try to clean up some of the more worrying warnings that are due to legacy code style.

@drsteve
Copy link
Contributor

drsteve commented Jul 5, 2022

I've revisited this and I have figured out the updates required on the spacepy side to have this work. Tests pass on my local, modified SpacePy version that uses the present updates. So basically, I don't see any major issues right now on this PR.

I will note that the items listed where "it happens in the following parts" are pretty ambiguous. For example, fly_in_nasa_aeap1 in AE8_AP8.f is updated, get_AE8_AP8_flux is not. It might make sense to document (in the PR and in the document if there were a couple of lists: one giving routines that have been changed to have no fixed upper limit of input array size; and one that gives those that still use the fixed upper limit. There's not really a good spot in the HTML documentation to list this sort of thing though. Maybe a CHANGELOG or a "release notes" document? I'm also going to leave a couple of minor comments as a review, and request changes. I would imagine this should be easy to tidy up and merge...

For my notes (for updating SpacePy to accommodate this change):
The wrapping itself updates very smartly, so we don't need to change that. So to accommodate this we can simply update spacepy.irbempy.prep_irbem to use an additional keyword argument (ntime_max) which can be set to either the IRBEM-allowed max, or None. If it's None then spacepy.irbempy.prep_irbem should set the input/output array dimensions to nTAI. Then the updated fortran subroutines that have no limit on number of times need to supply the new kwarg set to None. Then I can drop the ntime argument on calling the fortran subroutines that use it as the wrapping makes the dependent argument optional since it's just the length of iyear (or whatever variable is listed first when initializing in the fortran).

Copy link
Contributor

@drsteve drsteve left a comment

Choose a reason for hiding this comment

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

frames.html documents the arguments for many (all) of these subroutines, so the HTML docs should at least be updated to show the correct dimension.

It'd also be nice to have a place where things like this can be recorded, beyond just looking up a PR. Maybe a changelog or explicit release notes would be good. Maybe that's as simple as opening a new issue that describes the subroutines that can't be updated yet and what needs to change so the requirement for ntime_max can be dropped altogether.

Other than that, this looks good and is long overdue!

source/AE8_AP8.f Outdated Show resolved Hide resolved
source/AFRL_CRRES_models.f Outdated Show resolved Hide resolved
source/AFRL_CRRES_models.f Show resolved Hide resolved
source/AFRL_CRRES_models.f Show resolved Hide resolved
…imension everywhere, and updated documentation
@AntoineBrunet
Copy link
Contributor Author

So this is a long overdue PR. Significant changes were to be done to the documentation since a lot of interfaces change (hopefully in a backward-compatible manner). What I've done is to remove the mention of NTIME_MAX except in the few routines were they were necessary:

  • SGP4-related routines, for which the output arrays have variable dimensions
  • Radiation belt models (AFRL_CRRES and AE8/AP8), which have (NTIME_MAX, Nene) outputs.
  • Shell-splitting routines (make_Lstar_shell_splitting and LandI2Lstar_shell_splitting), which have (NTIME_MAX, Nipa) outputs.

@mshumko
Copy link
Contributor

mshumko commented May 16, 2024

Hi @AntoineBrunet,

Looks like this PR has a minor conflict with your recently merged docs PR. Once you resolve the docs/frames/frames.html conflict I will merge this PR as well.

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