Remove all associated with perturbation Jacobians about arts HOT 8 CLOSED

@riclarsson Here I was far too quick! Not possible to clean up as much as I claimed. Realised that there are variables that are solely handled by jacobian_agenda, and thus are "non-analytical". We have (at least)

• Polyfit and Sinefit
• Pointing (don't see how to do that analytically without making things very slow)
• FreqShift and Stretch (could potentially be made analytical, but then up to you)
  For the moment I just wanted to correct what I said. I need to digest this a bit before deciding exactly what to do.

from arts.

@erikssonpatrick It seems to me we should make a clean cut here based on how we actually treat different kinds of derivatives. The design now is very messy because you do not want to use different parts of jacobian_quantities in the same way (different "x2arts...") and this is not even well-documented everywhere.

Since this is basically how we want it to be, though, there should be a clean cut between the retrieval quantities instead of continuing the present mess. Have the three classes AtmosphericQuantity, ModelQuantity, and InstrumentQuantity all parts of a JacobianQuantities-class that holds a std::vector of each of these. iy-functions promises to fill the Jacobian to the best of their abilities for all relevant AtmosphericQuantity(s) and ModelQuantity(s). Later functions can be responsible for the InstrumentQuantity(s). It would be nice to separate the external derivatives from the internal ones anyways.

By such a redesign, you would also be able to get rid of some other design mistakes. Like some of the needless checks for the various agendas.

from arts.

@riclarsson I have now done the cleaning I planned and unassigned me.
As things work, I suggest that we close this issue. But I leave that decision to you.

@riclarsson A comment, just in case. One of the variables I removed from e.g. iy_main_agenda is nlte_field. Did not understand why it was an input. All tests work so I hope this OK. I mention this as there could be something in NLTE part is missed. In fact, I also tried to remove f_grid. That was OK for all tests, beside testRotationalConvergence.arts. It took me a while to understand that you create internal f_grids in these calculations and in the end, I left f_grid in all iy_xxx agendas for consistency and to handle possible future extensions. If you at some point need to make iterative calculations, where nlte_field is updated in each iteration, I assume we need to readd nlte_field.

from arts.

I will have a look at rearranging some things when I have more time but it is OK to close this issue.

As you say, I need to generate f_grid myself. This is because it would be a nightmare to demand the user provide a frequency grid themselves that integrates to 1 over frequency for all lines. It is simply easier to deal with this myself.

testRotationalConvergence requires updated nlte_field every call to the radiation agenda. The idea is to recompute the statistical equilibrium equation every time for a new radiation field and then repeat until the nlte_field converges. I did not believe it possible that the function was working while removing this input. However, there seems to be a bug in get_iy_of_background(), which is why nlte_field could be removed from the test case. Instead of removing nlte_field, nlte_field should be added to both iy_surface_agenda and iy_cloudbox_agenda.

Since I have always used pure blackbody surface emission in these tests (and I do not believe there will be much difference if I switch to other styles), I have simply never seen that both cloudbox and surface agendas are missing this input.

from arts.

@riclarsson Then my suspicion that nlte_field must be input was correct. I will add it to iy_main_agenda and iy_surface agenda. It does not make sense to add it to iy_cloudbox_agenda. Inside this agenda, you interpolate a pre-calculated doit_i_field.
Why do you think there is a bug in get_iy_of_background? Please give more detailed information.

from arts.

@erikssonpatrick

The bug is that get_iy_of_background is not taking nlte_field as input for the possible surface reflections. I must have missed adding this before, and I have since only used pure emission schemes. It will cause silent errors if more detailed choices for the surface agenda were made. Any agenda that does clearsky calculations need nlte_field as input to allow for general calculations.

I am not entirely sure what is done in iy_cloudbox_agenda. If there are zero clearsky calculations, it is fine to not have nlte_field as input.

from arts.

@riclarsson Yes. As iy_surface_agenda did not take nlte_field as input, get_iy_of_background did not need it. I will fix that.

from arts.

I close this now. The important things are done and all seems to work.

from arts.