Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MB hacks, required changes and brainstorming for latest master #115
base: main
Are you sure you want to change the base?
MB hacks, required changes and brainstorming for latest master #115
Changes from 13 commits
ce227a4
61f780a
f308152
ac687e3
c7486c8
6dfff49
a0a1066
c969e1d
ac9cbb5
fb4f448
76f2aae
902560d
85af75c
686e177
1e76764
87ad842
5bc8bf7
6351533
68d2c50
d27c852
c358a4c
6b3271d
01fa3e5
dd4c6e1
3649d35
0c34146
6afa7f7
f4cd4ed
07af4bd
72e45da
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan to have that hardcoded. If you want it globally selectable instead of putting short everywhere in shape definition, I can live with a KWK_DEFAULT_DIMENSION_TYPE thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right right, i have a lot of other changes incoming (still strugling with fully moving to new kiwaku, quite time consuming) so please wait a day or two until i push those - then will have to have another 'facetime' i'm pretty sure 🙈 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in advance: the main problem is that if we support heterogeneous shape containers, defaulting runtime-sized dimensions to a fixed integral type defined in joker is inconsistent/incomplete: one should be able to choose that also - for now i've put in an nttp based mechanism that most resembles what you do elswhere - so you'd say shape<_[short{}]> to get a 16bit untyped runtimesized axis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually this whole joker idea really 'broke my back', lost a lot of time on that - it made my code more complicated (as well as the changes i required in kiwaku) - the previous approach where a dynamic/runtime-specified dimension/axis was specified simply with a zero/default-constructed value (using/counting on the fact/assumption that no one will ever need or use a fixed zero statically sized axis - what would be the point of that? i.e. such a shape will always have a zero count/volume)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's already available (see https://jfalcou.github.io/kiwaku/glossary.html#glossary-extent)
https://godbolt.org/z/ov5r59jh5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I needed something regular for the other algorithms and shape based operation to work with less complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok great in one sense but in the other it makes it even more complicated :/ for example now i have to detect the as<> thingy in addition to jokers as 'dynamic axes'...
i've already spent entire days on this but will/am now changeing joker back to your initial design (almost :D) and moving to as<> thingies...will see how it goes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it made my 'stuff' non-trivially more complex (still haven't cleaned up all the hacks around it) - i guess will have to do a live session to sort it out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah as I don't have a view on your use cases, I can't take them into account.
The last batch of changes was to accomodate the variadic nature of shape and stride and do the heterogeneous requirements. It has been done so that the thing was comprehensible and had a regular, predictible behavior. So, now, if your specs changed or isn't aligned with that, I need some more informations or we'll be chasing aroudn each other for years and we both have other stuff on our respective plates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the change ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well as_array was suboptimal. I thought this was geenrting better code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think the main issue it that the way it was implemented actually did not work (compile) so this was a quick-fix...we'll have to go through all this live i guess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in which scenario did it din't work ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think when you used it on a shape that is only is_dynamic but not fully_dynamic (as the original constraint specified): you would index into a kumi tuple internal array that was shorter in length then the shape you were 'randomly' accessing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I see, our tests are then incomplete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same remark that for joker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be put in a macro in detail/abi.hpp ?
Also is it really necessary as axis are mainly used as constexpr entity ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i want the axis thingy to behave as a 'unit' wrapper (like boost.units)...wait for other changes..