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

Fix data initialization #81

Merged
merged 10 commits into from
Aug 11, 2020
Merged

Fix data initialization #81

merged 10 commits into from
Aug 11, 2020

Conversation

BenjaminRodenberg
Copy link
Member

@BenjaminRodenberg BenjaminRodenberg commented Aug 4, 2020

closes #80

This PR provides a draft (or a solution?) for #80.

Main changes

  • Include initialize_data in initialize
  • Make write_function optional argument of initialize
  • Raise assertion, if no write_function is provided, but self._interface.is_action_required(precice.action_write_initial_data())
  • We need some additional logic to be able to call coupling_expression.is_scalar_valued() or coupling_expression.is_vector_valued() on an empty coupling_expression. Empty means, created, but not updated.

Restrictions

  • we do not check self._interface.is_read_data_available() anymore.

Todo

Copy link
Member Author

@BenjaminRodenberg BenjaminRodenberg left a comment

Choose a reason for hiding this comment

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

I highlighted some of the obvious todos of the current draft.

fenicsadapter/fenicsadapter.py Outdated Show resolved Hide resolved
fenicsadapter/fenicsadapter.py Outdated Show resolved Hide resolved
fenicsadapter/fenicsadapter.py Outdated Show resolved Hide resolved
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.

Changes align the adapter to a better workflow for initialization 👍
Some open points to be discussed and implemented (see comments above)

Copy link
Member Author

@BenjaminRodenberg BenjaminRodenberg left a comment

Choose a reason for hiding this comment

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

Some comments from the discussion with @IshaanDesai and @uekerman.

We decided not add an self._interface.is_read_data_available() branch in read_data, since this will interfere with the CouplingExpression and PointSource update/creation.

In write_data we could optionally add the self._interface.is_write_data_required().

fenicsadapter/fenicsadapter.py Outdated Show resolved Hide resolved
fenicsadapter/fenicsadapter.py Outdated Show resolved Hide resolved
@BenjaminRodenberg BenjaminRodenberg mentioned this pull request Aug 6, 2020
5 tasks
@IshaanDesai
Copy link
Member

As we no longer return data from initialization and also initialize an empty coupling_expression we need slightly more QN-iterations in each time window than previous reference results. This result is known and was observed during the design overhaul.

New implementation:

bcs = [DirichletBC(V, u_D, boundary)]
coupling_expression = precice.create_coupling_expression()
bcs.append(.., coupling_expression)

while precice._is_coupling_ongoing:
   ...

Old implementation:

initial_data = read_data from preCICE before coupling starts
bcs = [DirichletBC(V, u_D, boundary)]
coupling_expression = precice.create_coupling_expression(initial_data)
bcs.append(.., coupling_expression)

while precice._is_coupling_ongoing:
   ...

@IshaanDesai IshaanDesai marked this pull request as ready for review August 7, 2020 13:20
@BenjaminRodenberg
Copy link
Member Author

... we need slightly more QN-iterations in each time window than previous reference results.

The reason for this is probably (and hopefully) 3cd2fb8, where I intentionally removed initialization to see 1) whether our new scheme can handle cases without initialization and 2) what happens with the number of QN iterations. I would not expect a regression from this PR. I will now prepare a PR to update the tutorials according to this PR and carefully check for any regressions.

As soon as the tutorials PR is ready I would suggest to merge everything (don't forget to remove the heat.py from this PR again, since it was only introduced for prototyping).

@BenjaminRodenberg
Copy link
Member Author

I prepared precice/tutorials#101 and checked that everything is working. I did not find anything that we have to worry about. So from my perspective this PR is ready to merge.

@IshaanDesai IshaanDesai merged commit bd800a7 into develop Aug 11, 2020
@BenjaminRodenberg BenjaminRodenberg deleted the i80-initialize-data branch October 10, 2020 18:07
BenjaminRodenberg added a commit to precice/tutorials that referenced this pull request Dec 7, 2020
BenjaminRodenberg added a commit that referenced this pull request Dec 21, 2020
* Copy /tutorials/HT/partitioned-heat/fenics-fenics form precice/tutorials at b3f92fc into this repository for development. Also add logs to be able to check for regression.

* Move initialize_data() into initialize()

* Skip initialization to see what happens.

* Always create empty CouplingExpression and PointSource, use update function for data initialization and update.

* Update tests.

* Simplifying point sources definition and use

* Further simplifying initialization and adding error messages

Co-authored-by: ishaandesai <ishaandesai@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Location of read_data in partitioned-heat correct?
3 participants