use EFITLCFSMask in FluxCoordToCartesian

[Mateasek]

FluxCoordToCartesian.evaluate returns a fixed value if the coordinate is outside the LCFS. To evaluate whether the point is inside/outside a psi normalised interpolator is used and value compared to 1. If the coordinate is outside the psin interpolator range, errors are returned.

This could be avoided if EFITLCFSMask was used to identify the position.

[CnlPepper]

This change could be made, however such points would be outside the domain of the equilibrium solution (outside the psi grid). It was also result in quite a performance hit as the EFITLCFSMask function performs a mesh inside-outside test.

[CnlPepper]

Adding a vector blend function would probably be the better way to solve the issue. The user can then chose to take the computational hit if they wish.

[Mateasek]

Would adding a vector blend function help in this case? What would be the mask function, if not the EFITLCFSMask? There should be some kind of check if the requested position is inside the psi 2d map.

What about adding classes for clamping input of vector functions? This would also remove the problem if the input of the return value of the map_vector2d was clamped to ranges of the equilibrium. This way the FluxCoordToCartesian would stay the same, which would be better, since it can be used outside the equilibrium class.

[CnlPepper]

The blend function would be used to switch between values inside the LCFS and another function outside. If the other function is not dependent on the psi grid then a coordinate outside the psi grid domain will never be requested.

Essentially:

BlendVector3D(FluxCoordToCartesian(...), ConstantVector3D(1,2,3), EFITLCFSMask())

The FluxCoordToCartesian method should only fail if the user is requesting a value for a coordinate that lies outside of the psi grid domain, i.e. outside the EFIT data set.

When designing Cherab we deliberately decided that it should never make a decision for a user regarding any data domain. User should always be explicitly required to address domain issues. This is done to ensure all data manipulation is visible in the user script, reducing the risk of unexpected behaviour leading to bad results.

[Mateasek]

If the BlendVector3D is done the same way as the current BlendXD functions, then the mask function(in you example the EFITLCFSMask) is called in every evaluate call. This would have the same performance hit you mentioned in you first comment, wouldn't it? The improvement would be that the FluxCoordToCartesian stays general. What about adding a set of boolean functions checking if a coordinate is whithin a specified domain? This could be useful in more applications, for example when trying to avoid calling an interpolator outside its domain?

[CnlPepper]

The difference is the user can choose to take the performance hit, it isn't forced on them in the FluxCoordtoCartesian object.

FluxCoordToCartesian was written for EFIT data, I'm not sure what you mean by staying general? It isn't meant to be a general purpose method.

[Mateasek]

I'm sorry, I'm a bit confunsed now. If it was meant for EFIT and if you are suggesting to do in map_vector2d the same as is done for the map2d method (which uses blending and inside_lcfs which is EFITLCFSMask) of the equilibrium, then you are only shifting the EFITLCFSMask call from FluxCoordToCartesian to map_vector2d and you are not giving user any choice, are you? Plus you are doubling the check of the coordinate being outside the lfcs, which is done in FluxCoordToCartesian also. By general I meant moving the check of being outside the psi normalised domain from FluxCoordToCartesian and leave the possibility of it returning errors I wanted to get rid of.

[CnlPepper]

Wait.... is the intention of this issue to highlight that map2d and map_vector2d are treating the data outside psi>1 differently? If that is the case, then yes they should be brought into alignment. Looking at map2d, it appears the blend approach with the inside lcfs attribute is used... in this case yes, despite the impact to performance both function should be behaving the same. BlendVectorXD classes will need to be created. These could perform a simple slerp (spherical linear interpolation) between the two vector fields according to a third scalar function.

Can I ask you to iterate the issue from the perspective of the user API rather than the function classes internal to the module. It is helpful to know what you are trying to achieve rather than highlighting a proposed change with no context.

[CnlPepper]

I've just gone though the code and the history in more detail. That psi test you have identified simply shouldn't be in that class at all. I don't understand why that approach was taken, as pointed out it completely fails outside the efit domain, and therefore introduces inconsistency into the API. I assume it was a kludge that accidentally made it into repo.

Ok so I think I know what the issue is and what you want. The solution is to revert the FluxCoordtoCartesian to its original state and then implement the blend vector functions. Once these are available, modify map_vector2d to mirror the implementation of map2d and it will solve the problem correctly.

[Mateasek]

Ok, now I think I understand.

I will revert the changes in FluxCoordtoCartesian which will remove the check of psi being inside/outside of lcfs. I will add blend vector functions and then modify map_vector2d.

Thanks @CnlPepper for looking into it and I'm sorry if I failed to communicate properly what I thought was the problem.

[CnlPepper]

@Mateasek could you take a look at the PR? I've made the changes to the equilibrium object, if you are happy I'll merge them.