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

Inconsistent documentation #556

Closed
riclarsson opened this issue Oct 28, 2022 · 1 comment
Closed

Inconsistent documentation #556

riclarsson opened this issue Oct 28, 2022 · 1 comment
Labels
bug Issues that make the package feel broken documentation Addition or update of the documentation

Comments

@riclarsson
Copy link
Contributor

Describe the bug
vmr_field is defined to have the shape [species, p_grid, lat_grid, lon_grid]. (We do not have species as a variable, so if this is meant to be abs_species, then that should be specified. This is not the current issue, it has the size of abs_species which should perhaps be documented as the fix is done for this issue)

lat_grid is set to empty when the atmosphere is 1D but otherwise must have a size.

lon_grid is set to empty when the atmosphere is 1D or 2D but otherwise must have a size.

This inconsistency is clearly nonsense.

vmr_field should never have the shape [species, p_grid, 0, 0]. It should have at minimum the shape [species, p_grid, 1, 1] to contain any data. So the documentation of vmr_field is wrong.

When the AtmFieldsCalc method takes a zero-sized vmr_field_raw, it outputs a vmr_field of the shape [0, 0, 0, 0], even though there are pressure points. By the documentation, the shape should be [0, p_grid, 0, 0]. By practical reasons the shape should be [0, p_grid, 1, 1]. This seems easiest to achieve by having both internal and external FieldFromGriddedField take the atmosphere_dim variable and fix the output accordingly.

Anyways, this inconsistency leads to a direct bug in DisortCalcIrradiance. It assumes that the shape is at least [species, p_grid, 1, 1], which would be consistent with standard assumption that there may be some data. However, this assumption is clearly wrong since a 0-shape for latitude and longitude means we can have no data in 1D and 2D atmospheres.

To Reproduce
Steps to reproduce the behavior:

  1. Checkout PR solar ref spectra in irradiance #554 by @j-petersen
  2. Run the test having compiled in a mode that activates ARTS_ASSERT
  3. See the error in matpack as the MatrixView cannot be created from the vmr_field

Expected solution
@m-brath and @erikssonpatrick you two need to discuss through how you wish to define the atmospheric grids here. Clearly the disort integration is wrong by current documentation. Clearly current documentation is also wrong as we need a minimum of some data for 1D and 2D calculations too. This comes together in also fixing the various ways that vmr_field can be generated so that it consistently checks the atmosphere_dim instead of just the gridded field array to determine the final size.

So two tasks:

  1. Ensure that documentation is consistent across the board wrt to the shape of output parameters. I am sure that the false documentation of vmr_field may also be present in other fields because of copy-paste errors.
  2. Decide how you solve this for Disort. We shouldn't need temporaries to work around these kinds of problems as long as the input is OK. At the very least, Disort should check all its input fields that they are OK for what it assumes to be a 1D atmosphere.
@riclarsson riclarsson added bug Issues that make the package feel broken documentation Addition or update of the documentation labels Oct 28, 2022
@erikssonpatrick
Copy link
Contributor

@riclarsson @m-brath @stefanbuehler @olemke @j-petersen
This time it is I that decides to broaden the discussion, as non-existing/active dimensions continue to create confusion. I brought up aa_grid for DOIT some time ago, and now Richard asks about lat_grid and lon_grid. We have not been consistent in ARTS around how to look at grids not actively used. I think user and coding perspectives clash here, and that is why things are inconsistent. So we need to sort this out once for all. And I think the solution is to agree on how we look at "extra" dimensions, and in particular, if the associated grid should have length 0 or 1.

Let's assume that we have a case where the variation in the "extra" dimension is totally undefined. I would say that for such cases, the natural choice (from a user perspective) is to expect an empty grid. Because if asking for a length 1 grid, what does it mean?

But do we actually have any cases as above? A 1D atmosphere in fact implies that things are constant with latitude and longitude. With 1D and no sun, the radiation field is constant with azimuth (i.e. not being undefined).

Because if things are constant, it makes more sense to say that the associated grid should have length 1. In fact, this is already used here and there in ARTS. The retrieval grids work in this way. And the same when defining sensor characteristics.

As Richard explained, having empty grids cause problems for the documentation and coding. While I argue that it could make more sense for the user. But the question is if we actually will have a case where we have basically undefined dimensions. We have gradually gone from 1D with unpolarised emission, to 3D with Sun and polarisation, so it is not strange if old sins exist. My new ppath scheme will imply a full 3D view on the atmosphere in all cases. Will it be the same thing for calculations of radiation fields, that there is a clear view on the azimuth direction in all cases? Or undefined dimensions elsewhere?

That is, for me a length 1 grid implies constant data in that dimension, while length 0 flags undefined. And I would prefer to keep it like this. But on the same time ask if we have reached the state where we always have a clear view on the missing dimensions and could consistently treat length-1 grid in all of ARTS (i.e. implying constant data)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues that make the package feel broken documentation Addition or update of the documentation
Projects
None yet
Development

No branches or pull requests

2 participants