MLP::getParameters() is not correct

[AlexHarker]

MLP::getParameters() is not correct

The weights and biases cannot return anything useful as they are not passed in by reference, so the method sets local versions of these only. Also, the extents need to match, so apparently (according to @weefuzzy this may need to be done with reset().

[g-roma]

Not sure I see the issue here, the weights and biases are passed as FluidTensorView instances, this is not a container itself but manages a pointer to the actual storage. The assignment operator then copies the data to the FluidTensor allocated by the caller, which may be a bit confusing, but is a pattern used generally in the code base. The sizes are obtained from the MLP instance itself, this method is used in FluidJSON to store the MLP parameters as JSON.

[AlexHarker]

I understand what you are saying, but this is currently not at all obvious from reading the code. Looking at asFluid() it is very hard to disentangle.

If you are confident that this is correct (and indeed the usage in FluidJSON.hpp and my own experience of using son with map would seem to confirm that) then I'm happy for this to be closed.

As a side issue there's definitely something to think about should you wish the code to be navigable and useable by mere mortals - even if that's just a comment on the methods that use this return pattern, as it's very unlikely (IMHO) even an experienced coder would understand what is going on.

[AlexHarker]

To. be more constructive - the simplest solution to the documentation issue (but one that likely @weefuzzy might rightly not be happy about) would be to rename types - if "Views" were "Ptrs" or something like that then this behaviour would become much less surprising from the viewpoint of the method, although the code within the method would still be non-obvious, as setting the value of a pointer type (the operation in the method is = ) generally doesn't result in changing the underlying data of the original pointer.

[weefuzzy]

Here obviously isn't the place for thrashing through alternative designs, but I will not that this source of confusion is more my fault than @g-roma's.

We all agree that having operator= do a deep copy is confusing (although this is also what Eigen::Map does, so at least we're consistently confusing). Our experience so far has been pretty clear that having some sort of operator to do a multi-dimensionally range-checked deep copy is tremendously handy, both for coding quickly and for catching all sorts of bounds errors early. Perhaps a different operator would be clearer, whilst retaining the convenience.

Name-wise, C++20 uses span for a pointer + size combination, and this class satisfies a concept called View (which FluidTensorView doesn't, due to the deep copying). My slightly more radical idea than simply renaming would be to look at the feasibility of getting rid of FluidTensorView altogether and instead using the reference implementation of a multi-dimensional span looking likely to be n C++23. It does almost everything we can currently do (and better, obvs). Even if that turns out not to be a goer, I'd be open to renaming and decoupling from FluidTensor (or even ditching the latter).

Anyway, I don't think there is a bug here? So, suggest we close and all can feel welcome to start new issues with framework design ideas.

[AlexHarker]

There is no bug. I agree that the operator= usage is the most confusing aspect of the reading the code and would support a name change for that operation. The other matters are likely for those more consistently involved, so I think this can be closed, and hopefully there is some useful contribution to possible areas of confusion, but I'll leave that to the main team to pick up - if input is helpful of course, please ask.