Skip to content

A draft CONTRIBUTING document for the EXP source tree [no ci]#79

Merged
michael-petersen merged 47 commits intomainfrom
devel
Jul 10, 2024
Merged

A draft CONTRIBUTING document for the EXP source tree [no ci]#79
michael-petersen merged 47 commits intomainfrom
devel

Conversation

@The9Cat
Copy link
Member

@The9Cat The9Cat commented Jul 2, 2024

Ported from the readthedocs version

@The9Cat
Copy link
Member Author

The9Cat commented Jul 7, 2024

Current status

  • Merged in bug fixes from createFromCache, including the new constructors, and moved the Koopman experimental extensions to a new branch KoopmanHankel.
  • Checked compilation of devel on Ubuntu 24.04 and on Unity.
  • Version is bumped to 7.7.99. We can tag this on merge to main.

@michael-petersen
Copy link
Member

At least on my machine (M1 Mac), sqrt is not a constexpr, so I was receiving and error:

EXP/expui/FieldBasis.cc:316:33: note: non-constexpr function 'sqrt' cannot be used in a constant expression
    constexpr double fac0 = 1.0/sqrt(4*M_PI);

This is fixable by allowing fac0 to be computed at runtime; I don't imagine this is a big problem, but perhaps there is a preferred fix? I think we dealt with this before, but I can't remember. I've pushed the simple change.

@The9Cat
Copy link
Member Author

The9Cat commented Jul 8, 2024

It turns out that this does violate the C++17 standard, even though it's available in GNU. I'd recommend the following workaround:

constexpr double fac0 = 0.25*M_2_SQRTPI;

This is better because it indicates that the quantity fac0 is determined at compile time, not run time.

@The9Cat
Copy link
Member Author

The9Cat commented Jul 9, 2024

We are probably close to merging this menagerie of fixes to main.

Copy link
Member

@michael-petersen michael-petersen left a comment

Choose a reason for hiding this comment

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

This PR fixes several small bugs, and contains a new-and-improved test scheme. As all tests pass (both on GitHub and in several local cases), I'm happy to approve these changes.

@michael-petersen michael-petersen merged commit cbd99ce into main Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants