FluidGrid: proposed dimensions output to the fitTransform method #32
Conversation
| @@ -32,6 +32,7 @@ class GridClient : public FluidBaseClient, OfflineIn, OfflineOut, ModelObject | |||
| using string = std::string; | |||
| using BufferPtr = std::shared_ptr<BufferAdaptor>; | |||
| using StringVector = FluidTensor<string, 1>; | |||
| using IndexPair = std::tuple<index,index>; | |||
There was a problem hiding this comment.
Note to other reviewers: this needs to be std::tuple here because the CCE wrappers can handle that as a message return, but not (yet) std::pair. If / when the wrappers are updated to account for pair, then this slight inconsistency w/r/t the algorithm could be ironed out
There was a problem hiding this comment.
Credits should be given: with @weefuzzy I would have never been able to do this and his patience was infinite as usual.
There was a problem hiding this comment.
another question, meta, to all reviewer: did I get the dependencies right? I put the PR here in core, and did only feature branches in the two CCE wrapper repos. I think it is clear enough and doesn't clutter the PR of the other 2 but I know @weefuzzy did differently for another one (the new rolling stat object).
| if (src.dims() != 2) return Error("Dataset should be 2D"); | ||
| if (src.size() == 0) return Error(EmptyDataSet); | ||
| if (src.dims() != 2) return Error<IndexPair>("Dataset should be 2D"); | ||
| if (src.size() == 0) return Error<IndexPair>(EmptyDataSet); | ||
| FluidDataSet<string, double, 1> result; |
There was a problem hiding this comment.
result doesn't make as much sense here now that there is actually a return value from the message. Also declaring and assigning like this means that we miss an oppurtunity for Return Value Optimisation (i.e the result of Grid::process() gets constructed directly into the FluidTensor in this scope). Would suggest
| FluidDataSet<string, double, 1> result; | |
| FluidDataSet<string,double, 1> destination = mAlgorithm.process(... |
There was a problem hiding this comment.
I like that as it frees result for the output indeed.
| FluidDataSet<string, double, 1> result; | ||
| result = mAlgorithm.process(src, | ||
| get<kResample>(), get<kExtent>(), get<kAxis>()); | ||
| destPtr->setDataSet(result); | ||
| return OK(); | ||
| IndexPair casted = mAlgorithm.getGridMax(); |
There was a problem hiding this comment.
and I would then call this result (this isn't really a cast, it happens to be a conversion). Did you try returning directly without the intervening temporary tuple?
| IndexPair casted = mAlgorithm.getGridMax(); | |
| IndexPair result(mAlgorithm.getGridMax()); | |
| return {result}; |
There was a problem hiding this comment.
I tried without the tuple and it moaned. I can retry now that everything else is working.
Co-authored-by: weefuzzy <[email protected]>
|
Removing normalization just to get the grid dimensions at the output makes a lot more sense in workflow. I like it. Also found I wanted the min/max and went searching for a method inside the object to grab it without doing dict-fu. I will test a build if its ready to go? |
|
Yes it is ready to go. Adding a getter for min/max of the actual data in a dataset is/was on the more fluffy horizon - a sort of dataset stat - we talked about that a certain time ago and could be useful as a method there too... or maybe instead of this PR I just propose as this would give us the info from the grid dataset itself... |
That is a better long term optimal solution and stops you have to query "up the chain" with recursive-y patch cables. |
|
Hello, I also remembered about the idea of getting a summary of dataset (as e.g. in the R language) with the initial proposal, which was to output the range of x and y. This makes more sense to me, since the rows and columns are specific to Grid, while the x and y ranges just follow the input unless the extent is specified. Having them as return value also makes sense. Code wise, I think the names getGridMax and casted seem misleading, I would use something like getGridDimensions and respectively gridDimensions or gridDims. and members should start with m (which is also missing in the original version for assign2D). Finally, I should say that the PR author deciding when it is "ready to go" seems to defeat the whole PR / code review process. |
I think PA just meant that it was ready to be build and played with / tested rather than it is ready to be mainlined.
This is a great idea |
|
OK, sorry, I thought you were planning to test for a release, in any case all the proposed changes so far would not change the proposed behaviour, although the code would need to be tested again of course. |
Very soon I imagine, although it is not a given that his will be in the next release. It does solve a workflow issue which stands out though. |
|
Gerard's suggestions were on point, especially about keeping the algorithm stateless if at all possible. I'll have a look in a bit |
I agree that is a better interface than every object reporting back its stuff in some imagined UX hell-scape. No stress, I only raised this because someone was asking me about how to use grid and they brushed up against this particular problem. I know I also did in workshops so its probably something good to tackle. If not this release another one. |
* Adjust cmake to handle changes to Max SDK from 8.2 onwards * correct typo
* incorporate Ted's changes and recommendations Revert "fix pre/post cmake" This reverts commit 7d04ffb. Create does-it-build.yml does it build? consume SDK curl the 8.0.3 SDK add a ls stage for sanity remove cmake flags for OSX architectures move list cmd add working directories for steps that urn in build use macos as runner use ninja for build process add specific name for build will write a text file named foo to the home directory see if we can headless max patchers also build on the ci branch Buf2List: tidy up and add a 'buffer' message for immediate processing Revert "fat binaries" This reverts commit 35ef645. Revert "build script" This reverts commit 1ee3f5a. # Conflicts: # create.sh Initialize dumpDictionary pointer in MaxWrapper explicitly, as this is assumed in destructor removed dangling script naming to buf2list corrected buf2list use in mlpclass corrected interface of mds to make it more interactive updated buf2list2buf helpfiles with doc dial back automatic patch testing in workflow * Adjust cmake to handle changes to Max SDK from 8.2 onwards (flucoma#32) * Adjust cmake to handle changes to Max SDK from 8.2 onwards * correct typo * Update macos-build-test.yml use the latest sdk from github * you actually have to git clone... * only build ampslice for now * test releasing * make names + commits * give a proper asset name * use ref for tag * Update macos-build-test.yml * spell prerelease properly * change convention for naming n stuff * Update macos-build-test.yml * add windows-build script * quotes around path for windows * test a composite workflow * update composite * make a nightly action * now check to see if we can build and see an external * update infrastructure * check with cached outputs too * pre check in the right place * dont check a non-existant file * remove nightly cache workflow * build and combine * remove bad line * do some sanity checks for cleaning * inspect what is going on * cleanup ls -R and change to latest macos * see if we can copy the entire externals folder * check * now build fat and see * remove mistake space * get uname * get clang version * get system info * try some silicon black magic * use custom core * use https * check if core is there * core is only one folder up * make a release again * make externals early * 🩴 :burger: * pass DCMAKE_APPLE flag * full install of max * make release builds on windows * remove pdbs from release-packaging * dont remove pdb files * fix paths because they have spaces 👿 * try and deal with the hell that is windows * now release it! * try again * only build on dev and ci branches * now with DDOCS=ON * ddocs off * provide a more detailed tag for nightly builds * also make docs * use blas compilation branch (temporarily) * fix malformed paths * missing -D * take max_ref and merge with final compile * delete nightly before remaking it * delete extraneous experimental patch Co-authored-by: James Bradbury <[email protected]> Co-authored-by: weefuzzy <[email protected]>
* incorporate Ted's changes and recommendations Revert "fix pre/post cmake" This reverts commit 7d04ffb. Create does-it-build.yml does it build? consume SDK curl the 8.0.3 SDK add a ls stage for sanity remove cmake flags for OSX architectures move list cmd add working directories for steps that urn in build use macos as runner use ninja for build process add specific name for build will write a text file named foo to the home directory see if we can headless max patchers also build on the ci branch Buf2List: tidy up and add a 'buffer' message for immediate processing Revert "fat binaries" This reverts commit 35ef645. Revert "build script" This reverts commit 1ee3f5a. # Conflicts: # create.sh Initialize dumpDictionary pointer in MaxWrapper explicitly, as this is assumed in destructor removed dangling script naming to buf2list corrected buf2list use in mlpclass corrected interface of mds to make it more interactive updated buf2list2buf helpfiles with doc dial back automatic patch testing in workflow * Adjust cmake to handle changes to Max SDK from 8.2 onwards (flucoma#32) * Adjust cmake to handle changes to Max SDK from 8.2 onwards * correct typo * Update macos-build-test.yml use the latest sdk from github * you actually have to git clone... * only build ampslice for now * test releasing * make names + commits * give a proper asset name * use ref for tag * Update macos-build-test.yml * spell prerelease properly * change convention for naming n stuff * Update macos-build-test.yml * add windows-build script * quotes around path for windows * test a composite workflow * update composite * make a nightly action * now check to see if we can build and see an external * update infrastructure * check with cached outputs too * pre check in the right place * dont check a non-existant file * remove nightly cache workflow * build and combine * remove bad line * do some sanity checks for cleaning * inspect what is going on * cleanup ls -R and change to latest macos * see if we can copy the entire externals folder * check * now build fat and see * remove mistake space * get uname * get clang version * get system info * try some silicon black magic * use custom core * use https * check if core is there * core is only one folder up * make a release again * make externals early * 🩴 :burger: * pass DCMAKE_APPLE flag * full install of max * make release builds on windows * remove pdbs from release-packaging * dont remove pdb files * fix paths because they have spaces 👿 * try and deal with the hell that is windows * now release it! * try again * only build on dev and ci branches * now with DDOCS=ON * ddocs off * provide a more detailed tag for nightly builds * also make docs * use blas compilation branch (temporarily) * fix malformed paths * missing -D * take max_ref and merge with final compile * delete nightly before remaking it * delete extraneous experimental patch Co-authored-by: James Bradbury <[email protected]> Co-authored-by: weefuzzy <[email protected]>
* copy the misc folder in installation target * Add relative path file writing for data objects In principle consistent with other Max objects (which aren't wholly consistent in edge cases) * reset points if dict is successfully parsed * crediting (flucoma#78) crediting the new team * update kdtree help file * add fluid.stats~ and change category to statistical analysis * move stft and add fluid.grid~ * grid out help file real estate * adds fluid.plotter to the autocomplete objectlist * adds fluid.waveform~ to objectlist * first tab beautified * reorder latency message * getlatency after changing preset * Transients Help File (flucoma#82) * grid out help file * update transients~ help file * ci/automatic building (flucoma#29) * incorporate Ted's changes and recommendations Revert "fix pre/post cmake" This reverts commit 7d04ffb. Create does-it-build.yml does it build? consume SDK curl the 8.0.3 SDK add a ls stage for sanity remove cmake flags for OSX architectures move list cmd add working directories for steps that urn in build use macos as runner use ninja for build process add specific name for build will write a text file named foo to the home directory see if we can headless max patchers also build on the ci branch Buf2List: tidy up and add a 'buffer' message for immediate processing Revert "fat binaries" This reverts commit 35ef645. Revert "build script" This reverts commit 1ee3f5a. # Conflicts: # create.sh Initialize dumpDictionary pointer in MaxWrapper explicitly, as this is assumed in destructor removed dangling script naming to buf2list corrected buf2list use in mlpclass corrected interface of mds to make it more interactive updated buf2list2buf helpfiles with doc dial back automatic patch testing in workflow * Adjust cmake to handle changes to Max SDK from 8.2 onwards (flucoma#32) * Adjust cmake to handle changes to Max SDK from 8.2 onwards * correct typo * Update macos-build-test.yml use the latest sdk from github * you actually have to git clone... * only build ampslice for now * test releasing * make names + commits * give a proper asset name * use ref for tag * Update macos-build-test.yml * spell prerelease properly * change convention for naming n stuff * Update macos-build-test.yml * add windows-build script * quotes around path for windows * test a composite workflow * update composite * make a nightly action * now check to see if we can build and see an external * update infrastructure * check with cached outputs too * pre check in the right place * dont check a non-existant file * remove nightly cache workflow * build and combine * remove bad line * do some sanity checks for cleaning * inspect what is going on * cleanup ls -R and change to latest macos * see if we can copy the entire externals folder * check * now build fat and see * remove mistake space * get uname * get clang version * get system info * try some silicon black magic * use custom core * use https * check if core is there * core is only one folder up * make a release again * make externals early * 🩴 :burger: * pass DCMAKE_APPLE flag * full install of max * make release builds on windows * remove pdbs from release-packaging * dont remove pdb files * fix paths because they have spaces 👿 * try and deal with the hell that is windows * now release it! * try again * only build on dev and ci branches * now with DDOCS=ON * ddocs off * provide a more detailed tag for nightly builds * also make docs * use blas compilation branch (temporarily) * fix malformed paths * missing -D * take max_ref and merge with final compile * delete nightly before remaking it * delete extraneous experimental patch Co-authored-by: James Bradbury <[email protected]> Co-authored-by: weefuzzy <[email protected]> * dont use BLAS branch (fixed in pr#67) * remove dangling line * use dev branch of flucoma core for ci * cleanup english and patching on tab 2 * deferlow on file load (was failing before) * update cosmetics * point to /reference/sines in the learn platform not ampslice * add learn and flucoma.org links * apply 10x10 grid * cancel previous runs if a new push is made * Only need 1 outlet * update outlet assistance * update outlet 1 assist * change tag name of nightly to be versionless * Remove per-PR merge checks * update bufselect * add links to top of file * initial cleanup * patcher for linking to learn articles more easily * changes to chroma help file * remove spaces from installation path name (flucoma#90) * fix references to spaced package name in CI * sorted the package-info with a few fun anachronicisms * missed a space in macbuild target * only do a release on dev branch * only do a release on dev branch * trigger build on PR * remove per PR building * remove release checking * add a deferlow to fluid.learn * provide a small musical example on first tab * add chroma frequency calculation * replace list.change with zl.change * human unit test maxpat * make a soundful demonstration on each tab * fix incorrect refrences to previous help files * update positioning of online reference/flucoma link * Fix fluid.learn abstraction not displaying properly (flucoma#101) * initial cleanup * changes to chroma help file * add a deferlow to fluid.learn * provide a small musical example on first tab * add chroma frequency calculation * replace list.change with zl.change * make a soundful demonstration on each tab * fix incorrect refrences to previous help files * update positioning of online reference/flucoma link * update fluid.learn patch to render more reliably * Load media files from package abstraction (flucoma#102) * a preliminary go at the bufloader * tighten regex and implement auto loading * add second tab for curating statistics with bufselect * add cosmetic upgrades to second tab and neaten first tab * fix loading woes with fluid.learn * more cosmetic upgrades to patch * cosmetic upgrades to chroma * [Review Changes] BufChroma Help File (flucoma#98) * update fluid.bufchroma~ help file * Fix fluid.learn abstraction not displaying properly (flucoma#101) * initial cleanup * changes to chroma help file * add a deferlow to fluid.learn * provide a small musical example on first tab * add chroma frequency calculation * replace list.change with zl.change * make a soundful demonstration on each tab * fix incorrect refrences to previous help files * update positioning of online reference/flucoma link * update fluid.learn patch to render more reliably * Load media files from package abstraction (flucoma#102) * a preliminary go at the bufloader * tighten regex and implement auto loading * stripping back of first tab * an interactive playback mechanism with chroma data * chroma lookup * add more detail tab * polish patch * layout fixes for help patch * [Review Changes] KDTree Help File (flucoma#92) * more cleanup on fluid.kdtree * separate tabs * add analysis data * cleanup tab 1 * add presliced corpus points * add a tab simplifying the explanation * cosmetic upgrades * add numbered instructions * [CI] Refactor and use composite actions (flucoma#106) * use flucoma actions * use composite actions to build windows * remove duplicate env setu * use maxdocs composite action * do CI on every ci located branch * Re-enable building docs as part of ALL * [CI] Fat Binaries (flucoma#107) * do CI on every ci located branch * bump to v3 * [CI] Actions@v4 (flucoma#108) * do CI on every ci located branch * use v4 of actions * update docs * Re-enable building docs as part of ALL * [CI] Fat Binaries (flucoma#107) * do CI on every ci located branch * bump to v3 * [CI] Actions@v4 (flucoma#108) * do CI on every ci located branch * use v4 of actions * update docs * indentation * remove duplicate * typo * [CI] Fix Documentation Build (flucoma#115) * fix ci docs component * make release all the time * use ubuntu latest for building docs * [Enhance] Update fluid.waveform~ (flucoma#116) * update to translational api layer * use new api in help file * buf -> buffer * alias indicesbuffer to addmarkers * update help to reflect internal api * singular for methods * update waveform help * fix pluarlisation of methods and arguments * update help file for fluid.waveform * various cosmetic upgrades (flucoma#103) * BufNNDSVD Help File (flucoma#88) * update bufnndsvd~ helpfile * add playback to the help file * [Docs] Fix Layout issues in BufChroma help (flucoma#117) * fixes layout issues of bufchroma * fix bufcompose load issue * Update fluid.sines~.maxhelp * update dataset help file (flucoma#120) * labelset cosmetics * remove frombuffer tab * [Docs] Blocking Help Tab (flucoma#121) * add back blocking attribute * add attribute blocking to bufnndsvd * add tab to bufstft * add tab to bufselect * change release action * [Docs] BufLoudness Help (flucoma#122) * griddify help file * remove js starter * experiment with waveform-based interface * first tab done (possibly) * finish multichannel tab * PAs changes * [Docs] Loudness Help File (flucoma#118) * shell out loudness second tab * cosmetic updates to second tab * update second tab * PA suggestions * typo * more typos * [Docs] Audiotransport Help File (flucoma#105) * work on audiotransport help file * add fluid.learn abstraction * cosmetic updates * [Enhance] Retrieve toy DataSets (flucoma#126) * pull datasets from the location specificed in PR 88 of core * pull datasets to misc * also add an abstraction for loading datasets * update help file with new asset names * add misc to ignore * copy from Data instead of DataSets * copy resources on cmake configure * moved patchers from misc to patchers * delete the files tracked by mistake * update bufchroma to have an adaptive number of chroma (flucoma#129) * AmpSlice Help File (flucoma#81) * griddify help file * in progress work for explaining thresholds and curves * update bpatchers for providing linkage to sites * update ampslice to be more friendly and clear * add margin to link helper bpatchers (so they display correctly on load) * update help file to accomodate bpatcher changes * make text box way longer than it needs to be. * update cosmetics * update with review advice * update learn subpatchers * shuffle cosmetics * update bufaudiotransport names (flucoma#131) * Update nightly.yaml Add workflow dispatch for manual launch * correct docs path variable in CMake * [Docs] Grid Help File (flucoma#110) * add datasets that can be reused for examples * update to fluid.grid helpfile * try and make learn load in the right order... * update aesthetics and separation of tabs * cosmetic upgrades * Revert "add datasets that can be reused for examples" This reverts commit 20f58a3e972867a7947ae6946d1862be67afd2f6. * update dataloader abstraction * fix layout with new dataset loader * Max buffer references weren't being freed safely (ASan) (flucoma#132) * update quickstart * [Fix] fluid.plotter~ clear label data (flucoma#135) * fix single quotes * remove old var * clear all of the data that we need to clear * re-order precedence of colouring (flucoma#136) Co-authored-by: weefuzzy <[email protected]> Co-authored-by: tremblap <[email protected]> Co-authored-by: Timo Hoogland <[email protected]> Co-authored-by: James Bradbury <[email protected]> Co-authored-by: Ted Moore <[email protected]>
As discussed previously, the new size of the FluidGrid process can be a challenge to retrieve, if only because it requires another object (FluidNormalize) and dumping it in a dict and retrieving a key.
As a weekend distraction for yours truly, I hereby propose to return the 2 maxima when the
fitTransformmethod is called. This is streamlined and takes into account that the FLuidGrid object is stateless, in effect a processor.Amended Max and SC help (and class def for the latter) also are on feature branches in their respective repos.