Skip to content

Fix CI errors#376

Merged
cpjordan merged 4 commits intomasterfrom
value_shape-fix
Dec 4, 2024
Merged

Fix CI errors#376
cpjordan merged 4 commits intomasterfrom
value_shape-fix

Conversation

@cpjordan
Copy link
Copy Markdown
Contributor

Fix various issues with testing:

  • ufl_element() -> ufl_function_space()
  • Fatal Python error: Aborted - TO BE FIXED
  • AttributeError: 'Cofunction' object has no attribute 'ufl_class' - TO BE FIXED

@cpjordan cpjordan marked this pull request as ready for review November 28, 2024 22:05
Copy link
Copy Markdown
Contributor

@stephankramer stephankramer left a comment

Choose a reason for hiding this comment

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

Thanks @cpjordan
So it might be I'm just misunderstanding - but I would have expected that for interpolated value size we should in fact be using value_size and not block_size. The latter being the n/o DOFs per node, which was what previously returned by value_size in which case it would in fact have been incorrect in Thetis previously. The difference would only appear for RT and BDM velocity fields which we don't use very frequently. I do agree/understand the changes to the OP2 kernel headers in utility.py and utility3d.py - these should indeed be block_size now.

Comment thread thetis/callback.py Outdated
Comment thread thetis/exporter.py Outdated
Comment thread thetis/callback.py Outdated
Comment thread examples/discrete_turbines/turbine_callback.py Outdated
@stephankramer
Copy link
Copy Markdown
Contributor

stephankramer commented Dec 2, 2024

- Interested in scalar/vector/tensor not DOF structure
- Also simplify to fs.value_shape - no need for ufl_element().value_shape
@cpjordan
Copy link
Copy Markdown
Contributor Author

cpjordan commented Dec 3, 2024

@stephankramer - thanks for reviewing and explanation. I had discussed the problems with @connorjward who found that value_size in the utility kernels was the cause of the failing tests, but I just replaced all instances assuming that the previous implementation in Thetis was correct - I should have looked into it more!

Updated based on the comments above.

Copy link
Copy Markdown
Contributor

@stephankramer stephankramer left a comment

Choose a reason for hiding this comment

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

Perfect, many thanks!

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