Skip to content

Add cylindrical and periodic mappers#388

Merged
jacklovell merged 11 commits intocherab:developmentfrom
vsnever:feature/cylindrical_and_periodic_mappers
Apr 6, 2023
Merged

Add cylindrical and periodic mappers#388
jacklovell merged 11 commits intocherab:developmentfrom
vsnever:feature/cylindrical_and_periodic_mappers

Conversation

@vsnever
Copy link
Copy Markdown
Member

@vsnever vsnever commented Nov 30, 2022

This solves #387 by extending the collection of Cherab mappers with PeriodicMapper#D, VectorPeriodicMapper#D, CylindricalMapper and VectorCylindricalMapper.

The periodic mappers are needed to handle the results of simulations carried out with periodic boundary conditions, while the cylindrical mappers are needed to handle the functions defined in cylindrical coordinates.

For example, with the new mappers a 3D function defined in cylindrical coordinates on a 20° toroidal sector can be mapped with:

CylindricalMapper(PeriodicMapper3D(function, 0, np.pi / 9, 0))

and a vector function can be mapped with:

VectorCylindricalMapper(VectorPeriodicMapper3D(vector_function, 0, np.pi / 9, 0))

@jacklovell
Copy link
Copy Markdown
Member

As we discussed today, these are really coordinate transforms (same dimensionality) rather than mappers (increase dimensionality). The difference is subtle but we should think about how to organise these: I think they should live in a separate submodule under cherab.math.

@jacklovell jacklovell added this to the 1.5 milestone Dec 22, 2022
@vsnever
Copy link
Copy Markdown
Member Author

vsnever commented Dec 23, 2022

I agree, let's move these to cherab.math.transform and rename to CylindricalTransform, PeriodicTransformXD etc.

@vsnever
Copy link
Copy Markdown
Member Author

vsnever commented Dec 28, 2022

The functions are renamed to PeriodicTransform#D, VectorPeriodicTransform#D, CylindricalTransform and VectorCylindricalTransform and moved to cherab.math.transform. The docs are updated to include the new cherab.math.transform sub-module.

@vsnever vsnever linked an issue Jan 25, 2023 that may be closed by this pull request
@jacklovell
Copy link
Copy Markdown
Member

Looks good. Can you add unit tests for the VectorPeriodic*Transform classes? The scalar classes have tests but the vector classes are missing them.

(By the way, test coverage can be checked by installing recent versions of Cython and Coverage and running ./dev/build.sh --line-profile && coverage run -m unittest && coverage report (with optional coverage html for easier navigation).

@vsnever
Copy link
Copy Markdown
Member Author

vsnever commented Mar 14, 2023

Thanks for the review, @jacklovell. I've added tests for the VectorPeriodicTransform functions.

@jacklovell
Copy link
Copy Markdown
Member

Thanks. Looks good to me now.

@jacklovell jacklovell merged commit 81cf653 into cherab:development Apr 6, 2023
@vsnever vsnever deleted the feature/cylindrical_and_periodic_mappers branch May 11, 2023 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add CylindricalMapper and PeriodicMapperXD

2 participants