-
Notifications
You must be signed in to change notification settings - Fork 462
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
Regression test and OpenFAST updates #42
Conversation
…s, given an attribute of interest
when executing the python script directly from its parent directory. for example, this was a breaking execution: python manualRegressionTest… but this was fine: python reg_tests/manualRegressionTest...
Unicode characters cause problems when dealing with multiple platforms and various software reading the output files
@@ -0,0 +1,168 @@ | |||
# - Returns a version string from Git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to add the original repo from where you got this file? Also are the licensing terms of the original repo/file compatible with moving it into OpenFAST repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sayerhs good point. that file is from this widely used github repo: https://github.com/rpavlik/cmake-modules. It is licensed under Boost Version 1.0. The original author and a link to the license are included in the docstring of the files included in our repo so I think we are in the clear legally. For reference, the first paragraph of the license is included below. And I'll add a link to the repo in that file's docstring.
Boost Software License - Version 1.0 - August 17th, 2003
Permission is hereby granted, free of charge, to any person or organization
obtaining a copy of the software and accompanying documentation covered by
this license (the "Software") to use, reproduce, display, distribute,
execute, and transmit the Software, and to prepare derivative works of the
Software, and to permit third-parties to whom the Software is furnished to
do so, all subject to the following:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rafaelmudafort Let's add a link to the git repo in the CMake file comments. That should suffice.
@@ -0,0 +1,41 @@ | |||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue with this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above
docs/source/install_linux.rst
Outdated
|
||
OpenFAST has the following dependencies: | ||
|
||
- LAPACK libraries provided through the variable ``BLASLIB`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an OpenFAST specific modification? Standard CMake variable is not BLASLIB
, I think that is Intel MKL variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gantech was this BLASLIB
variable only used for your script fast-build.sh
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I used it only for fast-build.sh and specifically to compile with mkl. I'm not sure if it was required. Is there another path forward?
-DCMAKE_INSTALL_PREFIX=$openfast_dir/install/ \
-DCMAKE_BUILD_TYPE=RELEASE \
-DBUILD_FAST_CPP_API=ON \
-DYAML_ROOT:PATH=$yaml_install_dir \
-DHDF5_USE_STATIC_LIBRARIES=ON \
-DHDF5_ROOT:PATH=$hdf5_install_dir \
-DLAPACK_LIBRARIES="$BLASLIB" \
-DBLAS_LIBRARIES="$BLASLIB" \
-DFPE_TRAP_ENABLED=OFF \
$EXTRA_ARGS \
../ &> log.cmake```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this edit:
OpenFAST has the following dependencies:
- LAPACK libraries
- For the optional C++ API, `HDF5 <https://support.hdfgroup.org/HDF5/>`__ (provided by ``HDF5_ROOT``) and `yaml-cpp <https://github.com/jbeder/yaml-cpp>`__ (provided by ``YAML_ROOT``)
- For the optional testing framework, Python 3+
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup... I agree. What is the Cmake standard for Lapack libraries though? May be I should be setting LAPACK_DIR=${BLASLIB}
in the sample install scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intel's BLASLIB
includes both -L
flags and -l
. My recommendation would be to clarify in the documentation that the users should set BLAS_LIBRARIES
and LAPACK_LIBRARIES
appropriately if the library isn't found in standard paths. And use BLASLIB
as an example when using Intel MKL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok... Can I submit a separate pull request reg. this then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Push to this branch/pull request (assuming Raf gives you write access) it is a simple doc fix anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gantech you should be able to submit edits to this pull request. lmk if not
New: OpenFAST-v0.1.0-555-g787e85a1-dirty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raf what is your plan for "Include OpenFAST 0.1.0 - OpenFAST 1.0.0 comparison"
@michaelasprague that will be a document listing the test cases that failed the regression test, documenting some of the major differences in solutions, and listing the procedure used for the regression test. This is very similar to the document I've prepared (though not yet fully shared) on the FAST v8 - OpenFAST 0.1.0 comparison. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There will be some further tidying up of the GIT hash-related code, but otherwise, I'm fine with these changes.
update regression test baseline solutions after pull request #42
Regression Test
OpenFAST
-- Including the git info in the module specific outputs is more involved since each module handles its own I/O and the I/O code is tucked into larger subroutines - saving for future work
TO DO BEFORE MERGE