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

Refactored data writing #13

Open
wants to merge 12 commits into
base: dev-partitioned-heat-equation
Choose a base branch
from

Conversation

boris-martin
Copy link
Contributor

(Merging on #1 as it relies on it, but deserves a separe discussion)

Fixed the bug where writing vector data failed. More generally, I rewrote the data writing code to be more flexible. Instead of deciding whether to look at DOFs or geometrical vertices then doing ugly conversion, we simply look where the preCICE vertices are located and evaluate (Function.eval()) the function at these points. So if at some point we decide to switch from mesh-based mapping to DOF-based mapping, nothing to change here :)

Also fixed a bug where determine_function_type returned the wrong result. New implementation is shorter and simpler.

I removed the old convert_fenicsx_to_precice function as new ones are used.

Tests were adapted, and now the write_vector_data test passes.

When calling eval(), we must provide the list of vertices and the cells where they live (e.g. "sample function at point (0.5, 0.7) assuming the point is in cell 14"). The cells are computed once at initialization, so actual evaluation is pretty efficient (there is no nearest-neighbor like search of the matching cell at each call).

Comment on lines +180 to +181
# TODO: fails for weird reasons.
#assert (write_function.function_space == self._write_function_space)
Copy link
Member

Choose a reason for hiding this comment

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

Lets figure this out before we merge this, it looks like we are messing up some types in DOLFINx

Copy link
Member

@IshaanDesai IshaanDesai left a comment

Choose a reason for hiding this comment

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

Some minor things still to be done. I did some formatting

@BenjaminRodenberg
Copy link
Member

Generally sounds like a good plan. One thing that alarmed me a bit:

When calling eval(), we must provide the list of vertices and the cells where they live (e.g. "sample function at point (0.5, 0.7) assuming the point is in cell 14").

You are talking about the following API here, right? scalar_function.eval([x,y,z],cell_id)

Why is this necessary? Is this the normal use of the eval() function or is this a special preCICE thing which makes using the function more complicated? I just imagine that this might higher the entry barrier for new users. Currently, I don't understand from where I should know the cell ID. Is there a way to also have a (less performant, of coure) eval() function that saves you as a user the additional work of finding out about the cell? Example signature: scalar_function.eval([x,y,z])

(after looking at the code I understood that the eval() function already requires the user to provide the cell, so it's not really an issue of this PR, so you can also ignore it here and we discuss this elsewhere, where it fits)

@boris-martin
Copy link
Contributor Author

@BenjaminRodenberg
I first wanted to do that, but I'm confident the Dolfinx API has no such things.

@BenjaminRodenberg
Copy link
Member

@BenjaminRodenberg I first wanted to do that, but I'm confident the Dolfinx API has no such things.

If that's a Dolfinx API thing, then Dolfinx users will also know how to deal with this situation (hopefully). I'm just from the good old FEniCS days, so it confuses me. But this probably just means I should learn more about Dolfinx ;)

precice_data.append(sampled_data[lid])
if space.num_sub_spaces == 0:
return FunctionType.SCALAR
elif space.num_sub_spaces == 2:
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be >= 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we support 3D, yes. Could also be simply "!= 0" as it's never 1 I think.

@BenjaminRodenberg
Copy link
Member

We should make sure to not forget about this PR. @PhilipHildebrand is currently working on the heat equation. As far as I understand the scope of this PR, it is not related to the heat equation, because this PR only deals with vector-valued quantities.

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.

4 participants