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

added functionality for replace_subject/test/trial when handling mixed spaces #322

Merged
merged 1 commit into from
Jan 13, 2023

Conversation

JHopeCollins
Copy link
Collaborator

The replace_* functions in labels.py are missing some functionalities that are needed when handling mixed spaces

  1. replace_subject should correctly handle the new object being a ufl.Indexed instead of a Function/tuple of Functions etc
  2. replace_test_function and replace_trial_function should be able to take an idx argument to replace single components of mixed spaces.

@JHopeCollins JHopeCollins self-assigned this Jan 12, 2023
@JHopeCollins JHopeCollins marked this pull request as draft January 12, 2023 17:52
@JHopeCollins
Copy link
Collaborator Author

JHopeCollins commented Jan 12, 2023

Some extra points:

  • I added Exceptions when you should have passed an idx to replace_subject but didn't.
  • When the parameter combination to the test_replace_subject unit test are incompatible it was previously returning True. pytest was complaining and saying that this would be deprecated soon, so I changed to return None. This could also be changed to calling pytest.skip(<some useful message>) from inside the test.
    I need to think about the parameter combinations because adding the function_or_indexed parameter has doubled the number of tests. Now I think I have a fix I can go back and see if all of these are necessary.
  • Should the replace_subject test be checking that the subject has been replaced correctly? I think that currently it just checks that it hasn't thrown.

@jshipton jshipton marked this pull request as ready for review January 13, 2023 10:28
@jshipton
Copy link
Contributor

This looks great - thanks @JHopeCollins! I have made an issue to remind us to improve the test for replace_subject: #323

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