Skip to content
Snippets Groups Projects

fixes to isolate the action of precomp.

Merged Lee McCuller requested to merge lee-mcculler/pygwinc:precomp_fix into master

Ok, now for maybe a more controversial push.

this whole notion of mutating the "ifo" struct during construction of the noise curves is bad news, but rampant. The "gwinc" call makes a good first start by doing a deepcopy. It seems to me that this deepcopy should be the job of the first function to mutate ifo. In this case precompifo should do the copy.

Furthermore, the beginnings of the "gwinc" function are really additional precomputations, so they have been factored into precompifo.

the F_Hz array is placed in ifo at this point since it already contains dhdl and suspension information that has used F_Hz.

This way future calls can have a simplified API that only needs ifo in the same way that dhdl is given this status.

Really I'm doing this so that optimizer work that wants to work on individual curves within the gwinc call can be made in a more sane fashion.

As a side note to the merge. there should be some other mechanism for this mutation of ifo. It will probably be fixed if we switch to an object-oriented approach to the noise curves. In that, precomputations can be provided by @property methods and memoized within them... Anyway this merge is an incremental palliative.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • I have no objection to this change, but a couple of small nit-picks first:

    • The import name of the plot module is changed, but I don't see the point of that change or how it relates to the moving of code to the precompIFO function.
    • I would like to stick to PEP8 style as much as possible, which includes spaces after '#' in comments and line wrapping at 79 characters.

    In general, I think patches should be as simple an focused as possible, and should respect the coding style already in place, or at least conform to PEP8 (I of course realize that much of the existing code does not conform to PEP8, and hopefully it will approach over time).

  • Author Maintainer

    The change to the plot module name was because it was clashing with the "plot" argument to the gwinc function, so one of them needed to be changed or we need to extricate those commands from the gwinc function as they are redundant with main (as my comment declares.

    currently it is a bit of a challenge to prevent any unrelated changes in commits since there are so many to be made.. This merge req for instance is just to preempt needs in a much more involved topic branch (which won't touch core much).

    I'm trying to keep pep8 and in general my editor points out the spacing stuff. It seems not to with the TODO, so I'll watch for it.

  • @lee-mcculler sorry, I missed that bit about the plot namespace collision. i just fixed that in master in a separate patch.

    I agree that the entire gwinc() function is a bit redundant. It's there in consideration for the matgwinc interface. I'm certainly fine to toss it out, since I almost certainly will never use it.

  • Lee McCuller added 21 commits

    added 21 commits

    Compare with previous version

  • merged

Please register or sign in to reply
Loading