No more precomp

This finally gets rid of the whole precompIFO function. It does this by breaking up precomp into various smaller functions, moving them into the common IFO noise definition module, and using them appropriately when needed. This does result in some of the functions being called multiple times in the full budget calculation for the aLIGO-like budgets, but the cost should be minor given the convenience of getting rid of the whole precomp thing, thereby opening up everything to non-aLIGO-like configurations.

Even though some functions are calculated multiple times, we're still orders of magnitude faster than matgwinc, so... Should still revisit down the line though.

closes #40 (closed)

added 1 commit

• 7e641f65 - remove precomp

Compare with previous version

changed the description

added 1 commit

• 334bf5d7 - remove precomp

Compare with previous version

mentioned in issue #40 (closed)

I'm a fan of getting rid of precomp. Especially when combined with the interface simplifications, this greatly simplifies the workflow.

One thing I do like about precomp though is using it to quickly look up interferometer properties, sometimes without ever making a noise budget. For example, to find the beam waist ifo.gwinc.BeamWaist.

It looks like the new code calls the relevant 'mini' precomp functions now in noises.py as needed when calculating each budget item. Sometimes the results are stored in the gwinc struct as the functions are called, which can lead to some bugs if the noises are defined in different orders in the init file for an ifo. For example, QuantumVacuum calls precomp_gwinc which then defines the ifo.gwinc struct and populates it with some parameters. So if QuantumVacuum is defined last, or not at all, functions that rely on that struct will fail.

Also, the beam radii are no longer calculated correctly. Some yaml files have BeamRadius properties for ITM and ETM, however with the old precomp this was unnecessary as whatever values were stored in ifo.Optics.ETM.BeamRadius and ITM.BeamRadius were overwritten by precompIFO. Indeed, some yaml files have incorrect values listed. It might be the case that the new arm_cavity precomp is called every time it's needed, but having possibly incorrect values of the beam radii loaded from the yaml files seems dangerous.

What I really like is not having to do the extra ifo = gwinc.precompIFO(ff, ifo) every time. Can we do something like having the new load_budget function do this automatically by default? I'm guessing part of the impetus for this change though was to have all of the calc functions in each noise class be independent. (Although as noted above, that is not currently the case.)

Thank you very much for the review, @kevin.kuns. You make some valid points, but I don't see any of them as deal breakers for this particular path forward right now. since I think the benefits of breaking things up this way still out weight the losses, I think we should push through with this MR as is, and continue to think about how to improve things going forward.

One thing I do like about precomp though is using it to quickly look up interferometer properties, sometimes without ever making a noise budget. For example, to find the beam waist ifo.gwinc.BeamWaist.

Yeah that's a nice convenience. Hopefully we can figure out a way to actually precompute all these precomp functions, without requiring that a single monolithic precomp function. Maybe by somehow "registering" them, as @sean.leavey suggested.

It looks like the new code calls the relevant 'mini' precomp functions now in noises.py as needed when calculating each budget item. Sometimes the results are stored in the gwinc struct as the functions are called, which can lead to some bugs if the noises are defined in different orders in the init file for an ifo. For example, QuantumVacuum calls precomp_gwinc which then defines the ifo.gwinc struct and populates it with some parameters. So if QuantumVacuum is defined last, or not at all, functions that rely on that struct will fail.

Actually, in this new setup, the Noise classes call the various "precomp" functions as needed, and don't assume that they have been previously called. This does introduce some redundancy, as some of the functions may be called multiple times in a single budget run (e.g. precomp_suspension), but at least it clears up the log jam of requiring a single monolithic precomp function to be called ahead of time. So for the precomp_gwinc example, it's actually only the noise.quantum.shotrad function that depends on the presence of the ifo.gwinc sub-struct, so precomp_gwinc is only called in the QuantumVacuum.calc() method. If it was needed elsewhere it would need to be called elsewhere.

This all points back to the "memoization" that I was saying that we still want to think about.

Also, the beam radii are no longer calculated correctly. Some yaml files have BeamRadius properties for ITM and ETM, however with the old precomp this was unnecessary as whatever values were stored in ifo.Optics.ETM.BeamRadius and ITM.BeamRadius were overwritten by precompIFO. Indeed, some yaml files have incorrect values listed. It might be the case that the new arm_cavity precomp is called every time it's needed, but having possibly incorrect values of the beam radii loaded from the yaml files seems dangerous.

Ok this sounds like a bug. I'll look into it.

Thanks!

Also, the beam radii are no longer calculated correctly. Some yaml files have BeamRadius properties for ITM and ETM, however with the old precomp this was unnecessary as whatever values were stored in ifo.Optics.ETM.BeamRadius and ITM.BeamRadius were overwritten by precompIFO. Indeed, some yaml files have incorrect values listed. It might be the case that the new arm_cavity precomp is called every time it's needed, but having possibly incorrect values of the beam radii loaded from the yaml files seems dangerous.

Ok this sounds like a bug. I'll look into it.

Actually I take this back. ifo.Optics.ETM.BeamRadius is not used anywhere in the new code, and the arm beam waist is always calculated from scratch, so I think all is good.

Actually, in this new setup, the Noise classes call the various "precomp" functions as needed, and don't assume that they have been previously called. This does introduce some redundancy, as some of the functions may be called multiple times in a single budget run (e.g. precomp_suspension), but at least it clears up the log jam of requiring a single monolithic precomp function to be called ahead of time. So for the precomp_gwinc example, it's actually only the noise.quantum.shotrad function that depends on the presence of the ifo.gwinc sub-struct, so precomp_gwinc is only called in the QuantumVacuum.calc() method. If it was needed elsewhere it would need to be called elsewhere.

I don't think this is entirely true. precomp_gwinc defines the ifo.gwinc struct and then populates it with some quantities computed by ifo_power. But then ITMCarrierDensity requires ifo.gwinc.finesse which will not be defined if this is called before QuantumVacuum. So ITMCarrierDensity should probably call ifo_power instead of relying on ifo.gwinc

If we go with this method, I suggest getting rid of the ifo.gwinc struct entirely.

In general, since

In this new setup, the Noise classes call the various "precomp" functions as needed, and don't assume that they have been previously called

the various "precomp" functions should probably avoid modifying the ifo class attribute directly. Otherwise the precomp functions can change the state and it will be possible to introduce bugs where the answer depends on the order in which the various Noise classes are called. This is related to the bug about the beam radii.

Also note that currently calling precomp_suspension in both SuspensionThermal and Seismic is unnecessary since precomp_suspension modifies the ifo class attribute directly and thus only needs to be called once.

Maybe the thing to do instead is to call the precomp functions as needed but have a check to see if that precomp function has already been called by another Noise class, and if it has don't call it again. I think this would be straightforward by checking if some parameter has already been calculated or some struct already defined. For example, to see if you should call precomp_gwinc

if not hasattr(ifo, 'gwinc'):
    precomp_gwinc(f, ifo)

This also solves the speed problem of calling functions multiple times.

added 23 commits

• 160b4a1d - 1 commit from branch gwinc:master
• fc9f476a - seismic code simplification
• 7395661f - single suspension seismic noise function
• e51c763a - coating noise simplification
• a6c0badc - substrate carrier density noise simplification
• 46a44368 - substrate thermorefractive noise simplification
• 41b266d9 - substrate brownian noise simplification
• 52ca1f8e - substrate thermoelastic simplification
• b9e30c99 - suspension thermal noise simplification
• 71f45606 - newtonian noise simplifications
• 306daaf2 - residual gas noise simplification
• f7e6ebb1 - quantum noise module code cleanup
• 929d79c8 - struct: fix comment
• 25e64190 - struct: add Struct equality magic method
• 014bcbf5 - struct: set default=None in Struct.get method
• 2aead2ea - struct: fix yaml loader
• d8cc08e4 - struct: Struct improve initialization signature to be more dict-like
• 364d4127 - struct: Struct incorporate all load_struct functionality
• 177a3e2e - nb: simplify BudgetItem freq attirbute access
• aa1f2a70 - nb: tweak budget str
• c52e4ee2 - nb: support multiple calibrations per noise
• 9a38ac76 - ifo: move strain conversion to be a Budget Calibration
• d7605524 - remove precomp

Compare with previous version

I don't think this is entirely true. precomp_gwinc defines the ifo.gwinc struct and then populates it with some quantities computed by ifo_power. But then ITMCarrierDensity requires ifo.gwinc.finesse which will not be defined if this is called before QuantumVacuum. So ITMCarrierDensity should probably call ifo_power instead of relying on ifo.gwinc

Ok this was just a bug. I'm pushing a fix now.

Also note that currently calling precomp_suspension in both SuspensionThermal and Seismic is unnecessary since precomp_suspension modifies the ifo class attribute directly and thus only needs to be called once.

I think you're missing the point here. The point is that we can't assume that it's already been modified, so each function needs to call the functions to update the ifo as needed. This is what I've been saying is the redundancy. But it's necessary in order to keep the noise calculations independent.

Maybe the thing to do instead is to call the precomp functions as needed but have a check to see if that precomp function has already been called by another Noise class, and if it has don't call it again. I think this would be straightforward by checking if some parameter has already been calculated or some struct already defined. For example, to see if you should call precomp_gwinc

No we can't do this, since the simple presence of the sub-struct does not indicate whether the dependent variables have been modified or not.

I don't think this is entirely true. precomp_gwinc defines the ifo.gwinc struct and then populates it with some quantities computed by ifo_power. But then ITMCarrierDensity requires ifo.gwinc.finesse which will not be defined if this is called before QuantumVacuum. So ITMCarrierDensity should probably call ifo_power instead of relying on ifo.gwinc

Ok this was just a bug. I'm pushing a fix now.

Ok this has been fixed. ITMCarrierDensity now calculates the finesse and no longer assumes ifo.gwinc exists.

added 1 commit

• e0dd0ef0 - remove precomp

Compare with previous version

added 1 commit

• 23251669 - remove precomp

Compare with previous version

marked as a Work In Progress

I changed this to a WIP until the !66 (merged) test update/overhaul has been merged, since that provides much better views of changes being made to noise traces (and in fact exposed some issues with the changes in this MR).

added 31 commits

• 23251669...b3adea0a - 15 commits from branch gwinc:master
• 8492a0d3 - seismic code simplification
• 3787ad71 - single suspension seismic noise function
• 5f230ef1 - coating noise simplification
• eee8a300 - substrate carrier density noise simplification
• f91c07b5 - substrate thermorefractive noise simplification
• 3a0fd2e1 - substrate brownian noise simplification
• edb7fcc2 - substrate thermoelastic simplification
• 080b9fb9 - suspension thermal noise simplification
• 25dcfa78 - newtonian noise simplifications
• 22063c14 - residual gas noise simplification
• 1597e718 - quantum noise module code cleanup
• 5fd28791 - nb: simplify BudgetItem freq attirbute access
• e9fbcf70 - nb: tweak budget str
• 1dd7b668 - nb: support multiple calibrations per noise
• fad427e9 - ifo: move strain conversion to be a Budget Calibration
• 38f764e9 - remove precomp

Compare with previous version

unmarked as a Work In Progress

All issues have been resolved and all tests pass, so this is ready to merge.

added 3 commits

• 7f532a07 - nb: support multiple calibrations per noise
• bdbbcc85 - ifo: move strain conversion to be a Budget Calibration
• c96bdbd6 - remove precomp

Compare with previous version

approved this merge request

approved this merge request

mentioned in issue #49 (closed)

merged