Skip to content

Conversation

@michael-petersen
Copy link
Member

This PR changes some asserts such that they don't throw clang warnings on Mac. It also adds a default to one enum, which was throwing another warning.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses clang compiler warnings on Mac by reformatting assert statements to use the standard C++ idiom assert(condition && "message") instead of the non-standard assert(("message", condition)) format. The PR also adds a default: case to an enum switch statement in BiorthBasis.cc to suppress another warning.

  • Reformatted 8 assert statements in CoefStruct.cc to use proper syntax
  • Added default case to DiskType enum switch statement

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
expui/CoefStruct.cc Reformatted all assert statements in deepcopy methods to use assert(condition && "message") format, eliminating clang warnings
expui/BiorthBasis.cc Added default: case to switch statement for DiskType enum to address compiler warning (introduces logic bug)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

michael-petersen and others added 3 commits December 3, 2025 11:36
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Member

@The9Cat The9Cat left a comment

Choose a reason for hiding this comment

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

Seems fine to me overall. I'll try to remember not to use the comma operator in asserts but, tbh, I have no idea why this is wrong. Seems like valid C++ to me.

…king; the request is _on_ by default and is _off_ for non-biorthogonal field classes.
@The9Cat
Copy link
Member

The9Cat commented Dec 4, 2025

The use of a virtual member function rather than a base-class variable will allow more complex Unit situations specific to derived classes in the future.

@michael-petersen
Copy link
Member Author

I've run all the pyEXP-examples now and all use cases we support there work with these changes. If you are happy, @The9Cat , we can merge to devel (and then devel up to main).

@The9Cat
Copy link
Member

The9Cat commented Dec 5, 2025

I will check exp-examples. I think I found some inconsistencies with devel last time I checked. I realize that this nobody probably uses that repo, but still, let's make it clean.

@The9Cat
Copy link
Member

The9Cat commented Dec 7, 2025

The Zang-disk velocity field example in pyEXP-examples is producing all zeros in the field quantities. The disk-halo example is not zero but is producing density fields that make no sense. I'll have to take a look. I can't see how Unit would have broken that, and afaict, the Unit info is correctly appearing in the HDF5 coefficient file, as designed. So not sure what's up yet.

Probably should add those notebooks to the repo soonish so other can test. I suppose I was shying away from adding more PRs, but these should be there, I think.

Separate issue: Docker images seem fine.

@The9Cat
Copy link
Member

The9Cat commented Dec 8, 2025

Fixed the velocity field issue. I had changed the call signature to match those of the other Basis classes but forgot to change that in OutVel.

I'm going to make a branch with a PR in pyEXP-examples to contain to EXP tutorial examples that exercise velocity fields. We do not have to accept this PR, but these notebooks might replace the existing ones, if we want.

@michael-petersen michael-petersen merged commit dbbf2ed into devel Dec 10, 2025
8 checks passed
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.

3 participants