Skip to content
Snippets Groups Projects

convert ID columns from ilwd:char to int_8s

Merged Kipp Cannon requested to merge (removed):ilwdchar_to_int into master
  • changes are confined to string formatting in XML I/O routines, there is no binary API change associated with this (IDs have always been stored internally as integers, not ilwd:char strings).
  • this has been announced a number of times, especially to the compact objects working group, and no objections have been raised. the gstlal team is ready to proceed with this conversion at this time.
Edited by Ghost User

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
  • Kipp Cannon changed the description

    changed the description

  • Karl Wette added lalinspiral lalmetaio + 1 deleted label

    added lalinspiral lalmetaio + 1 deleted label

  • Looks fine to me, @jolien-creighton Any concerns?

    Will this require changes in other packages? e.g. glue, ligolw?

  • Author Contributor

    glue.ligolw is incompatible with this, it will be left unmodified to serve as a compatibility suite for interacting with old documents (and will become abandonware). The new ligo.lw library already matches this format, and users who wish to interact with lal (an example being gstlal) will need to switch their codes from glue.ligolw to ligo.lw. Apart from changing the import, no other code changes are required. I will be preparing the gstlal patch this week, but we don't need to wait for that to be ready. It would, however, be good if I could remind the compact objects group one last time that this is happening before the branch is merged.

  • OK, sounds good. Unless @jolien-creighton has objections I'm happy with you merging this when you're ready.

  • I'm fine with this being merged when @kipp.cannon is ready.

  • mentioned in merge request bayeswave!60 (merged)

  • Kipp Cannon added 8 commits

    added 8 commits

    • 42d2b060 - stringutils: syntax changes
    • 6e56bb0b - Move the heterodyned pulsar signal model functions from LALApps into LALPulsar
    • 4d44ffe8 - Update default ephemeris files for Weave
    • 26854f09 - Fix PulsarParametersWrapper.py so that values like H0 and PHI0 can be returned as class items
    • 54a18fd0 - miscellaneous bit rot from Kipp:
    • fd51b0aa - add missing ligo-segments and python-ligo-lw dependencies
    • 32358b88 - convert ID columns from ilwd:char to int_8s
    • 2018acaf - port many python codes to ligo.lw

    Compare with previous version

  • Author Contributor

    I was reminded by Alex Pace yesterday that I had completely forgotten about the Python codes in lalsuite, the patch was only updating C codes. I've put two additional patches into the change set in the merge request: one to add missing python-ligo-lw and python-segments (don't know why that was missing) dependencies to various packages in lalsuite, and another to port python codes (not PE or legacy inspiral). for some reason git.ligo.org thinks there are 8 patches and they have merge conflicts ... that is not the case at my end, so I'm going to have to figure out what's going on here ...

  • Kipp Cannon added 13 commits

    added 13 commits

    • 2018acaf...fd11cee4 - 10 commits from branch lscsoft:master
    • 5e2ecc19 - add missing ligo-segments and python-ligo-lw dependencies
    • 77cf5772 - convert ID columns from ilwd:char to int_8s
    • af34d489 - port many python codes to ligo.lw

    Compare with previous version

  • Author Contributor

    sorted it out. the diff now shows the changes contained in the 3 patches.

  • Author Contributor

    sorted it out. the diff now shows the changes contained in the 3 patches.

  • Author Contributor

    OK, now the tests are failing because python-ligo-lw isn't installed in the test environment. I don't know how to fix that.

  • It needs to be added to the list of dependencies in the spec file and debian/control. However as ligo-lw has python-lal as a dependency that will create a circular dependency. There are two options:

    1. remove code that requires lal from ligo-lw.
    2. remove code that requires ligo-lw from lal.
  • Author Contributor

    but, as I said, that's what one of the three patches does: adds the dependencies to the .spec and debian/control files. it has not had the effect of getting python-ligo-lw installed. also, I don't understand the concern about the cycle, what is the consequence of that? note that there is no build-time cyclic dependency. one can build and and install either from source without having the other installed, and that is correctly indicated in the build requirements of both packages, neither of which lists the other.

    is what's going on here that the build environment is somehow only consulting "build requires" entries to decide what to install, and then after compiling it's attempting to run unit tests without subsequently installing the run-time dependencies?

  • I'm not sure I've followed this entirely. The number of dependencies in LAL should be kept to the minimum. If it doesn't create a build-time error then perhaps it should not be a "hard" check in configure -- the relevant code can just be disabled (try ... except around the import). But if we do want an additional dependency for LAL then we need to consider that separately. The bar is probably lower for other parts of lalsuite.

  • But I suppose if this is just replacing glue with ligo then that's not going to be a showstopper.

  • The problem is that python-ligo-lw is calling routines from python-lal and python-lal is calling routines from python-ligo-lw. Granted this is only a run-time dependency but as we run the testsuite as part of the build process this essentially makes them build-time dependencies as well. We've had this problem for a while when this code was part of glue but now python-ligo-lw explicitly adds a dependency on python-lal. This change essentially make python-lal and python-ligo-lw inseparable.

  • If they're inseparable, could they not be the same thing? I.e., have ligo-lw part of LAL?

  • That would make this problem go away, or split out LIGOTimeGPS and the cache routines into a separate lower level library.

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
Please register or sign in to reply
Loading