Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix default file format when using legacy netcdf #2062

Merged
merged 19 commits into from
Jan 7, 2021

Conversation

ZedThree
Copy link
Member

No description provided.

@ZedThree ZedThree added bugfix small-change Changes less than 100 lines - should be quick to review labels Jul 24, 2020
@ZedThree
Copy link
Member Author

@bshanahan Please could you check this works for you?

@dschwoerer
Copy link
Contributor

I am not sure legacy netcdf is the best name.
In some cases it would be nice to have "netcdf" as "any netcdf" - for example in the bout-config file (but also #2010)

It also breaks on ubuntu 18.04, failure from test-delp2:

        Option diffusion:useFFT = true (Command line)
Setting boundary for variable n
        core region:    Option all:bndry_all = dirichlet (data/BOUT.inp)

        sol region:     Option all:bndry_all = dirichlet (data/BOUT.inp)

        Option input:transform_from_field_aligned = true (default)
        Option input:max_recursion_depth = 0 (default)
        Option n:function = gauss(x-0.5, 0.2) * gauss(z - pi, 0.2*pi) (data/BOUT.inp)
        Option n:scale = 0.1 (data/BOUT.inp)
        Option all:evolve_bndry = 0 (default)
        Option n:evolve_bndry = 0 (default)
        Option datadir = data ()
        Option dump_format = nc (default)
Error encountered
====== Exception path ======
[bt] #6 ./test_delp2(+0x2cf2a) [0x562f7da9bf2a]
_start at ??:?
[bt] #5 /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xe7) [0x7fc7cefd8b97]
__libc_start_main at /build/glibc-2ORdQG/glibc-2.27/csu/../csu/libc-start.c:344
[bt] #4 ./test_delp2(+0x28972) [0x562f7da97972]
main at /usr/include/c++/7/bits/unique_ptr.h:821
 (inlined by) main at /home/IPP-HGW/dave/soft/BOUT-dev/tests/integrated/test-delp2/test_delp2.cxx:30
[bt] #3 ./test_delp2(+0x16bb2d) [0x562f7dbdab2d]
Solver::setModel(PhysicsModel*) at /home/IPP-HGW/dave/soft/BOUT-dev/src/solver/../../include/bout/physicsmodel.hxx:90
 (inlined by) Solver::setModel(PhysicsModel*) at /home/IPP-HGW/dave/soft/BOUT-dev/src/solver/solver.cxx:92
[bt] #2 ./test_delp2(+0x164fd7) [0x562f7dbd3fd7]
PhysicsModel::postInit(bool) at /usr/include/c++/7/bits/basic_string.h:211
 (inlined by) ?? at /usr/include/c++/7/bits/basic_string.h:737
 (inlined by) ?? at /home/IPP-HGW/dave/soft/BOUT-dev/src/physics/../../include/options.hxx:581
 (inlined by) PhysicsModel::postInit(bool) at /home/IPP-HGW/dave/soft/BOUT-dev/src/physics/physicsmodel.cxx:113
[bt] #1 ./test_delp2(+0x8d077) [0x562f7dafc077]
std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > Options::withDefault<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) at /usr/include/c++/7/bits/basic_string.h:211
 (inlined by) ?? at /usr/include/c++/7/bits/basic_string.h:220
 (inlined by) ?? at /usr/include/c++/7/bits/basic_string.h:647
 (inlined by) ?? at /home/IPP-HGW/dave/soft/BOUT-dev/src/mesh/../../include/bout/../boutexception.hxx:28
 (inlined by) std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > Options::withDefault<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) at /home/IPP-HGW/dave/soft/BOUT-dev/src/mesh/../../include/options.hxx:430
====== Back trace ======
 -> PhysicsModel::postInit on line 94 of 'physicsmodel.cxx'

====== Exception thrown ======
Inconsistent default values for 'dump_format': 'nc' then 'h5'

Can we rename so that netcdf is "any netcdf" and then maybe have a better name then legacy - otherwise if there is a new netcdf version it will be confusing.

@dschwoerer
Copy link
Contributor

I have theses netcdfs installed:

libnetcdf-c++4/bionic,now 4.2-8 amd64 [installed,automatic]
libnetcdf-cxx-legacy-dev/bionic,now 4.2-8 amd64 [installed]
libnetcdf-dev/bionic,now 1:4.6.0-2build1 amd64 [installed]
libnetcdf13/bionic,now 1:4.6.0-2build1 amd64 [installed,automatic]
netcdf-bin/bionic,now 1:4.6.0-2build1 amd64 [installed]
python3-netcdf4/bionic,now 1.3.1-1 amd64 [installed]

@ZedThree
Copy link
Member Author

Maybe something like bout::build::has_netcdf (== any netcdf) and bout::build::use_legacy_netcdf. I'd prefer to stick with "legacy", as it's the official name for that release: https://www.unidata.ucar.edu/software/netcdf/release-notes-cxx-4.2.html

I do want to eventually remove the legacy version completely -- it's almost ten years old now! Plus maintaining two separate versions is not great.

That error is because we've repeated setting the default file format. I'll fix that.

Also, just to clarify the different versions that are available, because the naming is a bit of a mess:

  • libnetcdf13/libnetcdf-dev is the C API, v4.6.0 -- latest is v4.7.4
  • libnetcdf-c++4/libnetcdf-cxx-legacy-dev is the legacy C++ API, v4.2, provided for backwards compatibility shortly before the release of v4.2 of the C API, when they moved the Fortran and C++ APIs out of the main library
  • libnetcdf-c++4-1/libnetcdf-c++4-dev is the "new" C++ API, v4.3.0 in Bionic -- latest is v4.3.1

Other distros may call the C++ API something like netcdf-cxx4 . In BOUT++, the legacy version is called NcFormat, and the new one is Ncxx4

@bshanahan
Copy link
Contributor

Still broken for me.

@ZedThree
Copy link
Member Author

Please could you post the error message and/or config.log? I'm afraid I don't recall what the original issue was

`datadir` and `dump_format` are set in `BoutInitialise`, so must
already have values. Setting a default value is either redundant or
inconsistent
@bshanahan
Copy link
Contributor

Sorry, the original problem is that configure did not recognize netcdf, so switched to h5. This has been fixed. I got confused because today I mentioned on slack that my code breaks if I try and collect while a simulation is running. This PR does not fix that problem.

* next:
  always pull fedora from fedoras registry
  Allow to override packages in fedora rawhide
  Delete removed test_io.grd.nc file from test-invpar's CMakeLists.txt
  SurfaceIter iterate over non-guard cells by default
  Formatting changes
  Invpar fixes for non-uniform dy
  CI: Turn off patch coverage check
  Prefer references to pointers
  Add variation of g_22 metric, dz != 0
  Remove commented-out 'grid=' line in input file for test-invpar
  Replace ccoef=3 test in test-invpar
  Use non-integer dcoef in test-invpar to avoid zero-pivot error
  Use FFT z-derivatives for test-invpar
  Don't use grid file for test-invpar
  Remove incorrect dz factors in InvertParCR
  Add z-variation to input of test-invpar to properly test z-derivs
  Correct implementation of Grad2_par2 in InvertParCR
  Check location of coefficients in InvertParCR
  Minor optimization of cyclic InvertPar implementation
  Change typo in curvature section
@ZedThree
Copy link
Member Author

bout::build::has_netcdf is now true for either interface. I've kept bout::build::has_legacy_netcdf to distinguish between the two.

Also (hopefully) fixed the problem @dschwoerer reported with test-delp2 by just removing the default values (& upgrading to the new Options API at the same time, as it's much cleaner)

Removing the redundant default options in physicsmodel affects things
that don't set the default options... My comeuppance for not using
BoutInitialise!
* next: (61 commits)
  Fix links to open-mpi.org and lam-mpi.org
  CMake: Add option to skip generating fieldops
  CMake: Make generated_fieldops.cxx command more portable
  CMake: Add module to find clang-format
  Suggestions for gnu build on Marconi
  Minor tidying of test-io_hdf5 runtest (including black formatting)
  Update the benchmark file for the hdf5 version of test-io too
  Minor tidying of test-io runtest (including black formatting)
  Update test-io benchmark file for new FieldPerp default attribute
  Add CMake code to generate generated_fieldops
  Fix comparison
  Make sure test-restart-io sets exit code correctly on failure
  Don't read/write FieldPerps in guard cells
  Use last boundary point for global y-index in restart IO test
  Test attributes of written variables in test-io and test-io_hdf5
  Better fix for reading/writing FieldPerp index
  Rename *local -> *global argument in mesh index routines
  Remove deprecated YLOCAL() in examples/laplace-petsc3d
  Fix case-sensitivity of __contains__()
  __eq__() comparison operator for BoutOptions()
  ...
bendudson
bendudson previously approved these changes Dec 28, 2020
@johnomotani
Copy link
Contributor

I checked the setup in #2182 again, and it's skipping lots of tests with netcdf => False. I configured just with ./configure, and in the summary got

configure:   NetCDF support          : no (legacy: yes)

I'd expect the netcdf condition to be true, even though it's legacy netcdf.

`BoutInitialise` ensures these are set, so only an issue if that isn't called
@ZedThree
Copy link
Member Author

ZedThree commented Jan 4, 2021

@johnomotani Please can you check now?

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

src/bout++.cxx Show resolved Hide resolved
@johnomotani
Copy link
Contributor

Unfortunately not, I still get the same symptoms as my previous comment. Here's the config.log in case that helps.

* next: (301 commits)
  GHA: Increase test timeout to 6 minutes
  Apply black formatting to test-squash
  Ignore some variables when comparing squashed outputs in test-squash
  Update expected results of test-io_hdf5
  clang-tidy suggestion
  Fix typo
  clang-tidy fixes
  Use bout_type="string" for strings in H5Format
  Write descriptions for std::vector<int> and std::string variables
  Fix reading of char* in Ncxx4
  Use MPI_C_BOOL type instead of C++ binding MPI::Bool
  Manual improvements for hypnotoad
  Removing hypnotoad grid generator
  clang-tidy suggestion
  Support location in Mesh::get()
  Fix a couple of clang-tidy warnings
  Completely remove LaplaceShoot
  CMake: Set PYTHONPATH for tests
  Remove some other references to 3D metrics
  Convert test-backtrace runtest to Python
  ...
@ZedThree
Copy link
Member Author

ZedThree commented Jan 5, 2021

I found a better way of testing it on my machine -- this should actually work now!

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

src/bout++.cxx Show resolved Hide resolved
src/bout++.cxx Show resolved Hide resolved
@johnomotani
Copy link
Contributor

Ah, now it's trying to compile Ncxx4 and OptionNetCDF because BOUT_HAS_NETCDF is true, and failing because it only has legacy netCDF...

@ZedThree
Copy link
Member Author

ZedThree commented Jan 6, 2021

🤦 I guess this worked for me because the netcdf-cxx4 headers are in a default search path, so the compiler could find them and it worked. I uninstalled it completely and the latest commit should actually, really, work this time!

@johnomotani
Copy link
Contributor

There are some tests for OptionsNetcdf that need to be switched off too: tests/unit/sys/test_options_netcdf.cxx and tests/integrated/test-options-netcdf. The unit tests would be easy, just change like 047ffa7, but the integrated test I think needs us to be able to do #requires: not legacy_netcdf, which isn't currently available.

@johnomotani
Copy link
Contributor

With a couple more tiny changes, this now works for me. I had to skip test-io, which is expected to fail with legacy netcdf. Also needed to add brackets to the #requires in test-options-netcdf, then realised that we don't need to check for Travis any more, and the test does pass on Fedora, so removed those checks.

@ZedThree ZedThree merged commit 8178252 into next Jan 7, 2021
@ZedThree ZedThree deleted the legacy-netcdf-build-config branch January 7, 2021 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix small-change Changes less than 100 lines - should be quick to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants