Update LORENE library

Create issue
Issue #1817 closed
Frank Löffler created an issue

The current Lorene thorn isn't up to date with the version that is available from Meudon. A new version of a thorn containing a new version of Lorene is available here:

https://bitbucket.org/einsteintoolkit/lorene.git

Diffs aren't available in the repo, but are attached as file here. That is quite large, but mostly due to necessary changes in the patches contained in the thorn (because the underlying files changed, not because the patches would have changed in functionality).

Normaly, updates like this would not be an issue, but this update of Lorene changes (breaks) old Lorene files. This is nothing we could decide upon, this is the Meudon group's decision. I nevertheless propose to update to the new version. This would need to include updating the testsuites of depending thorns, which might mean re-creating their ID files.

Keyword: parma

Comments (32)

  1. Frank Löffler reporter
    • removed comment

    At phone call: we should provide two different thorns for the different versions.

  2. Frank Löffler reporter
    • changed status to open
    • removed comment

    Suggestion: for the release, include this thorn (in another repository) as LORENE2 - but disabled due to the current inability to compile together with LORENE.

  3. Frank Löffler reporter

    We'll not include another thorn for the release, not even disabled in the thornlist - since we don't have a good way of testing it, nor is any thorn in the ET currently using it. That will change after the release.

  4. Frank Löffler reporter
    • changed status to open
    • removed comment

    I've updated the description to include the new repository URL. This is, at least for now, to be included in the ET thornlist as 'Lorene2'. The implementation name is, by design, 'Lorene', to allow easier replacement of the old thorn, and to make sure users only compile in on of both. Thorns using this library can use the version-mechanism in Cactus to make sure what Cactus provides is what they need, or let Cactus croak otherwise.

    Thorns that use this new version will follow, but an initial review, pending those, would still be welcome.

  5. Frank Löffler reporter
    • changed status to open
    • removed comment

    note: This is an external library. While documentation might be nice, the old version lacks that as well. And it wouldn't make sense to distribute a test suite with this thorn in any case (only with thorns that use Lorene). Lorene is used here as import-library mainly (although this thorn will also compile and provide the binaries to actually run Lorene, as Cactus utilities).

  6. Roland Haas
    • removed comment

    Not having looked into this any further, I'd like to re-iterate my comments on this in different ET calls: since this breaks backwards compatibility to the old LORENE code (it cannot read files written by the old code. Does the LORENE C++ API also change?) I would prefer if the thorn was called LORENE2 and also implemented LORENE2. Ideally it should be able to compile both LORENE and LORENE2 into the same executable. If not possible (since we don't control the names of LORENE's externally visible symbols) then one could at least try that one at least receives an error before the final link stage (or not at all since the linker normally just ignore duplicate symbols [without --whole-archive]).

  7. Frank Löffler reporter
    • removed comment

    Does the LORENE C++ API also change?

    Yes. The updated version, e.g., uses namespaces for at least part of the code. The old didn't. But it quite likely also does not use them for everything, so you do not want both versions compiled into the same binary.

    Ideally it should be able to compile both LORENE and LORENE2 into the same executable.

    I agree. Unfortunately, this seems to be a bad choice in this case.

    If not possible then one could at least try that one at least receives an error before the final link stage

    Both providing the same implementation (but different versions) would do exactly that. Cactus does not allow two identical implementations. This would be caught already at CST stage.

    It is already sub-optimal to provide two different versions of otherwise the same library. I don't want to introduce a new implementation name for every potentially new version of Lorene. I would strongly prefer the same implementation name, and use versions to make sure that thorns using Lorene within Cactus get the versions they actually work with.

  8. Frank Löffler reporter
    • removed comment

    In addition, I believe we should prepare a plan to phase out the old version of Lorene. Lorene is a living code. It evolves, and will likely continue to do so. We cannot support every possible version of Lorene in the long run.

    We have to come up with a plan how to incooperate changes in Lorene into the ET. In this sense, it would even be better to rename the current LORENE thorn to something else (maybe LORENE_2015 or something similar), and keep the LORENE thorn as close to the current lorene code as possible.

  9. Roland Haas
    • removed comment

    I am not sure of phasing out is possible since the new code does not read old data files. Unless there is a converter for the data files this would render old files unusable and would mean that groups cannot reproduce old results. Note that being able to use the new LORENE code to re-solve for the same set of physical parameters does not help since it would also involve a new code and thus different results (or new bugs).

    For the same reason I do not think that renaming is an option instead the new LORENE should be named LORENE2 same as CarpetRegrid and CarpetRegrid2.

  10. Frank Löffler reporter
    • removed comment

    I am not sure of phasing out is possible since the new code does not read old data files. Unless there is a converter for the data files this would render old files unusable and would mean that groups cannot reproduce old results.

    It wouldn't. It is still possible to read those using the old readers. You would use the same code that you used to reproduce old data. It's not as if old releases would go away. And if you really care about reproducibility, wouldn't you start with a Lorene input file, and not a Lorene output file?

    For the same reason I do not think that renaming is an option instead the new LORENE should be named LORENE2 same as CarpetRegrid and CarpetRegrid2.

    That is not quite the same. It is quite conceivable that future changes in Lorene would break backward compatibility again and again. It cannot be our goal to support LORENE, LORENE2, LORENE3, and so on. What we can, is support the version that was current at that time. If an older version is required - well, then it shouldn't be hard to dig out an old release.

  11. Frank Löffler reporter
    • removed comment

    Note that I got the thorn Meudon_Bin_NS to work with both versions of Lorene. It would still not work to read data generated with one version of Lorene with the other version of Lorene. The Lorene "checkpoint" file format changed, and there is nothing we can do about that.

  12. Roland Haas
    • removed comment

    There is no diff or similar of the non-LORENE tarball part of the code, is there? In particular the ccl files, build script, patches etc. If you can easily provide this that would help (the easier to review, the more reviews will happen). It seems that if you use "diff -u" (or just git diff which defaults to that) then trac will highlight the changes visually making it easier to review as well. Of course it could also be that the file needs to have an extension .patch instead of .diff (since the example I looked at had a .patch file).

  13. Frank Löffler reporter
    • removed comment

    I attached a diff of the interface (not of the actual Lorene changes).

  14. Roland Haas
    • removed comment

    In line "36 unset LIBS" if gfortran is required, what about ifort? In line "125 # build some utilities available with the LORENE library" shouldn't the copying of utilities be done via the makefile where UTIL_DIR is defined so that eg it is re-created after wiping the exe directory (minor)? In line "13 $(SCRATCH_BUILD)/done/$(THORN): $(SRCDIR)/build.sh " you are removing the tarball and the patches from the build dependencies. Did this not work before? Otherwise it would seem good to re-build LORENE when the tarball or the patches are changed.

    My comments with regards to thorn and implementation naming still apply.

  15. Frank Löffler reporter
    • removed comment

    Replying to [comment:20 rhaas]:

    In line "36 unset LIBS" if gfortran is required, what about ifort?

    Good catch. I added those as well, and tested with both. I don't like a white list like this in general, but linking both fortran and c++ seems to be messy like this, and we cannot simply take all LIBS from Cactus.

    In line "125 # build some utilities available with the LORENE library" shouldn't the copying of utilities be done via the makefile where UTIL_DIR is defined so that eg it is re-created after wiping the exe directory (minor)?

    Ideally it should do it later, but external libraries are pruned even inside the build phase, so one would need to create a temporary location, or alternatively, unpack part of the tarball again. I wanted to avoid this complication.

    In line "13 $(SCRATCH_BUILD)/done/$(THORN): $(SRCDIR)/build.sh " you are removing the tarball and the patches from the build dependencies. Did this not work before? Otherwise it would seem good to re-build LORENE when the tarball or the patches are changed.

    Good catch. This was a left-over from the time when this wasn't a tarball. It's in again.

    My comments with regards to thorn and implementation naming still apply.

    Yes, those will need to be figured out as well. I still think that using the same implementation name is ok, especially keeping in mind that now Meudon_Bin_NS works with both versions (but obviously not at the same time). The only issue to sort out is the thorn name, but the ticket isn't the right place to do so.

  16. Frank Löffler reporter
    • removed comment

    For the moment, it will be called LORENE2. This is not ideal, and should change in the future. However, for the sake of getting it into the release, albeit disabled, LORENE2 is ok for this release.

  17. Roland Haas
    • changed status to open
    • removed comment

    Fine with me to include then. At this point, please only as a download but not as a built-by-default thorn (since it conflicts with the regular LORENE due to duplicate symbols doesn't it?).

  18. Roland Haas
    • removed comment

    @knarf: I just tried loading "old" LORENE data files into the "new" LORENE and it does not seem to fail and differences on the grid are "smallish".

    Since you say in the description

    Normaly, updates like this would not be an issue, but this update of Lorene changes (breaks) old Lorene files. can you elaborate on this ie. under which circumstances does the file format change is this something that will fail loudly or silently give incorrect results?

  19. Frank Löffler reporter
    • removed comment

    The Lorene group decided to change the binary format. Files written by the old Lorene can only be read by that version, and files written by a new Lorene can only be read from that version.

    If it fails, it will complain loudly (abort the import).

  20. Roland Haas
    • removed comment

    Given that I just loaded a file produced by "old" LORENE in "new" LORENE and it did not fail as far as I can tell, can you provide more details?

  21. Frank Löffler reporter
    • removed comment

    Well, when I tried, it failed, if I remember correctly due to a difference in the c++ class members, which is an issue because Lorene uses c++ functionality to save whole classes. If it did work for you my best guess would be that it did use different classes, although I guess you also used a BNS? Maybe the EOS also plays a role. In any case, no, I don't have a failing-test case handy right now; I wished I did now.

  22. Roland Haas
    • removed comment

    Ok. Mine was a piecewise polytrope. I guess I better double check that I had the correct "old" and "new" versions. Thanks for the details. At least knowing that is fails loudly makes me feel a lot better already.

  23. Log in to comment