Bug fixes for hack insights #1032
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixed the template matching logic.
…into aasim/insights/fixes
kburtram
approved these changes
Jul 30, 2020
There was a problem hiding this comment.
I made a few of the same fixes, so you may need to merge and fix up (sorry :smile). I don't know about the change in transform as it seems to not follow the intended design for choosing the grouping column. Did you check with Nara on this, as it's possible I misunderstood the intended logic there?
| int minDistinctValue = -1; | ||
| int minColumnIndex = -1; | ||
|
|
||
| int maxDistinctValue = Int16.MaxValue; |
There was a problem hiding this comment.
should this be Int32.MaxValue? also, a common pattern for max is to set to the minimum possible value and then not have the extra check in the loop assuming all values will be greater.
There was a problem hiding this comment.
Yeah, I worked with Nara yesterday to change the logic of the slicer column select. The thought process behind this was that the categories(slicers) should have less distinct values than the input
| } | ||
| } | ||
|
|
||
| labels[stringColumns[minColumnIndex].ColumnIndex] = "input_g_0"; | ||
|
|
||
| labels[maxColumnLabelIndex] = "input_g_0"; |
There was a problem hiding this comment.
I thought the expected logic is that the string column with the least amount of distinct values should be the grouping column. Why are we making this change?
…into aasim/insights/fixes
kburtram
added a commit
that referenced
this pull request
Sep 3, 2020
* InsightsGenerator project template files * Add insights projects to SLN * Setting up siggen class (#1003) * Setting up siggen class * fixed the learn method * Refactoring code Fixed compile errors * renamed results to result * Basic transformation logic (#1004) * Fix a couple bugs and add a simple test (#1007) * Fix a couple bugs and add a simple test * More tests and bug fix * Nara/workflow (#1006) * added a queue processor * ordered using statements * Armemon/analytics (#1008) * Basic transformation logic * changed some structure of siggen * added sum and average method, as well as select rows by input name * add insights to results * min, max added Co-authored-by: Karl Burtram <karlb@microsoft.com> Co-authored-by: Aasim Khan <aasimkhan30@gmail.com> Co-authored-by: Arslan Memon <armemon@microsoft.com> * Added rules engine base implementation (#1005) * Added rules engine base implementation * update comments * addressing comments * adding template text to columnheaders object * adding template text to columnheaders object * fixing columnheaders class * Added test * Added Template Parser unit test in Test project * Deleted unnecessary files and reverted the files that were modified by mistake Co-authored-by: Jinjing Arima <jiarima@microsoft.com> * Insights generator message handler placeholder (#1013) * Aasim/insights/insight methods (#1014) * Basic transformation logic * changed some structure of siggen * Added top and bottom insight functions * Added top, bottom insights Added tests for top, bottom insights * Armemon/insights2 (#1011) * max and min insightsperslice, and tests * got rid of unneccesssary function * get indexes Co-authored-by: Arslan Memon <armemon@microsoft.com> * Armemon/insights2 (#1012) * max and min insightsperslice, and tests * got rid of unneccesssary function * get indexes * learn for stringinputtyype * add learn implentation Co-authored-by: Arslan Memon <armemon@microsoft.com> * Added Tests Removed duplicate methods Co-authored-by: Karl Burtram <karlb@microsoft.com> Co-authored-by: arslan9955 <53170027+arslan9955@users.noreply.github.com> Co-authored-by: Arslan Memon <armemon@microsoft.com> * Armemon/insights2 (#1016) * Basic transformation logic * changed some structure of siggen * Added top and bottom insight functions * Added top, bottom insights Added tests for top, bottom insights * max and min insightsperslice, and tests * got rid of unneccesssary function * get indexes * learn for stringinputtyype * add learn implentation * add unique inputs * fix merge error * add to result Co-authored-by: Karl Burtram <karlb@microsoft.com> Co-authored-by: Aasim Khan <aasimkhan30@gmail.com> Co-authored-by: Arslan Memon <armemon@microsoft.com> * Added all the templates (#1015) * Added all the templates Added a method to find matched template * Added a function to replace # and ## values in a template * Added ReplaceHashesInTemplate call * Added comments * Updated the template txt * Updated GetTopHeadersWithHash function to add #toplist * Updated the logic per offline discussion with Hermineh * Update request handler contract to take array (#1020) * added rulesengine findmatchingtemplate (#1019) * Add support for getting DacFx deploy options from a publish profile (#995) * add support for getting options from a publish profile * update comments * set values for default options if they aren't specified in the publish profile * addressing comments * Updating to latest DacFx for a bug fix (#1010) * added rulesengine findmatchingtemplate * Update DacFx deploy and generate script with options (#998) * update deploy and generate script to accept deployment options * add tests * add test with option set to true * merge * merge * incorporated FindMatchedTemplate Co-authored-by: Kim Santiago <31145923+kisantia@users.noreply.github.com> Co-authored-by: Udeesha Gautam <46980425+udeeshagautam@users.noreply.github.com> * -Added logic for Insights Generator Service Handler (#1017) * -Added logic for Insights Generator Service Handler * Fixed some logic * Adding workflow test * Update transform and add tests (#1024) * Jiarima/fix rules engine logic (#1021) * Added all the templates Added a method to find matched template * Added a function to replace # and ## values in a template * Added ReplaceHashesInTemplate call * Added comments * Updated the template txt * Updated GetTopHeadersWithHash function to add #toplist * Updated the logic per offline discussion with Hermineh * Update with the fixes * Updated template and foreach conditions * Added distinct * Updated tests according to the logic change (#1026) * Nara/remove queing (#1023) * loc update (#914) * loc update * loc updates * Add support for getting DacFx deploy options from a publish profile (#995) * add support for getting options from a publish profile * update comments * set values for default options if they aren't specified in the publish profile * addressing comments * Updating to latest DacFx for a bug fix (#1010) * Update DacFx deploy and generate script with options (#998) * update deploy and generate script to accept deployment options * add tests * add test with option set to true * intermediate check in for merge, transformed not working * intermediate check in for merge, transformed not working * added test case * merged Co-authored-by: khoiph1 <khoiph@microsoft.com> Co-authored-by: Kim Santiago <31145923+kisantia@users.noreply.github.com> Co-authored-by: Udeesha Gautam <46980425+udeeshagautam@users.noreply.github.com> * Output data types from transform (#1029) * Fix bug process input_g (#1030) * Fixed the insight generator service (#1028) * Jiarima/added more testings (#1031) * Added another test Updated ReplaceHashesInTemplate function to return string instead of Template * Added third test * Merged * Reverted the workflow file to match with the one in hack/insights * Bugs fixes to hook insights up to ADS (#1033) * Bug fixes for hack insights (#1032) * Fixed the minColumn index bug in Data Transformation Fixed the template matching logic. * Adding changes from PR * Try to fix Workflow tests * Readd workflow tests * Fix template load location Co-authored-by: Aasim Khan <aasimkhan30@gmail.com> Co-authored-by: Nara <NaraVen@users.noreply.github.com> Co-authored-by: arslan9955 <53170027+arslan9955@users.noreply.github.com> Co-authored-by: Arslan Memon <armemon@microsoft.com> Co-authored-by: gadudhbh <68879970+gadudhbh@users.noreply.github.com> Co-authored-by: Jinjing Arima <jiarima@microsoft.com> Co-authored-by: jiarima <68882862+jiarima@users.noreply.github.com> Co-authored-by: Kim Santiago <31145923+kisantia@users.noreply.github.com> Co-authored-by: Udeesha Gautam <46980425+udeeshagautam@users.noreply.github.com> Co-authored-by: khoiph1 <khoiph@microsoft.com>
nofield
pushed a commit
that referenced
this pull request
Jul 19, 2022
* InsightsGenerator project template files * Add insights projects to SLN * Setting up siggen class (#1003) * Setting up siggen class * fixed the learn method * Refactoring code Fixed compile errors * renamed results to result * Basic transformation logic (#1004) * Fix a couple bugs and add a simple test (#1007) * Fix a couple bugs and add a simple test * More tests and bug fix * Nara/workflow (#1006) * added a queue processor * ordered using statements * Armemon/analytics (#1008) * Basic transformation logic * changed some structure of siggen * added sum and average method, as well as select rows by input name * add insights to results * min, max added Co-authored-by: Karl Burtram <karlb@microsoft.com> Co-authored-by: Aasim Khan <aasimkhan30@gmail.com> Co-authored-by: Arslan Memon <armemon@microsoft.com> * Added rules engine base implementation (#1005) * Added rules engine base implementation * update comments * addressing comments * adding template text to columnheaders object * adding template text to columnheaders object * fixing columnheaders class * Added test * Added Template Parser unit test in Test project * Deleted unnecessary files and reverted the files that were modified by mistake Co-authored-by: Jinjing Arima <jiarima@microsoft.com> * Insights generator message handler placeholder (#1013) * Aasim/insights/insight methods (#1014) * Basic transformation logic * changed some structure of siggen * Added top and bottom insight functions * Added top, bottom insights Added tests for top, bottom insights * Armemon/insights2 (#1011) * max and min insightsperslice, and tests * got rid of unneccesssary function * get indexes Co-authored-by: Arslan Memon <armemon@microsoft.com> * Armemon/insights2 (#1012) * max and min insightsperslice, and tests * got rid of unneccesssary function * get indexes * learn for stringinputtyype * add learn implentation Co-authored-by: Arslan Memon <armemon@microsoft.com> * Added Tests Removed duplicate methods Co-authored-by: Karl Burtram <karlb@microsoft.com> Co-authored-by: arslan9955 <53170027+arslan9955@users.noreply.github.com> Co-authored-by: Arslan Memon <armemon@microsoft.com> * Armemon/insights2 (#1016) * Basic transformation logic * changed some structure of siggen * Added top and bottom insight functions * Added top, bottom insights Added tests for top, bottom insights * max and min insightsperslice, and tests * got rid of unneccesssary function * get indexes * learn for stringinputtyype * add learn implentation * add unique inputs * fix merge error * add to result Co-authored-by: Karl Burtram <karlb@microsoft.com> Co-authored-by: Aasim Khan <aasimkhan30@gmail.com> Co-authored-by: Arslan Memon <armemon@microsoft.com> * Added all the templates (#1015) * Added all the templates Added a method to find matched template * Added a function to replace # and ## values in a template * Added ReplaceHashesInTemplate call * Added comments * Updated the template txt * Updated GetTopHeadersWithHash function to add #toplist * Updated the logic per offline discussion with Hermineh * Update request handler contract to take array (#1020) * added rulesengine findmatchingtemplate (#1019) * Add support for getting DacFx deploy options from a publish profile (#995) * add support for getting options from a publish profile * update comments * set values for default options if they aren't specified in the publish profile * addressing comments * Updating to latest DacFx for a bug fix (#1010) * added rulesengine findmatchingtemplate * Update DacFx deploy and generate script with options (#998) * update deploy and generate script to accept deployment options * add tests * add test with option set to true * merge * merge * incorporated FindMatchedTemplate Co-authored-by: Kim Santiago <31145923+kisantia@users.noreply.github.com> Co-authored-by: Udeesha Gautam <46980425+udeeshagautam@users.noreply.github.com> * -Added logic for Insights Generator Service Handler (#1017) * -Added logic for Insights Generator Service Handler * Fixed some logic * Adding workflow test * Update transform and add tests (#1024) * Jiarima/fix rules engine logic (#1021) * Added all the templates Added a method to find matched template * Added a function to replace # and ## values in a template * Added ReplaceHashesInTemplate call * Added comments * Updated the template txt * Updated GetTopHeadersWithHash function to add #toplist * Updated the logic per offline discussion with Hermineh * Update with the fixes * Updated template and foreach conditions * Added distinct * Updated tests according to the logic change (#1026) * Nara/remove queing (#1023) * loc update (#914) * loc update * loc updates * Add support for getting DacFx deploy options from a publish profile (#995) * add support for getting options from a publish profile * update comments * set values for default options if they aren't specified in the publish profile * addressing comments * Updating to latest DacFx for a bug fix (#1010) * Update DacFx deploy and generate script with options (#998) * update deploy and generate script to accept deployment options * add tests * add test with option set to true * intermediate check in for merge, transformed not working * intermediate check in for merge, transformed not working * added test case * merged Co-authored-by: khoiph1 <khoiph@microsoft.com> Co-authored-by: Kim Santiago <31145923+kisantia@users.noreply.github.com> Co-authored-by: Udeesha Gautam <46980425+udeeshagautam@users.noreply.github.com> * Output data types from transform (#1029) * Fix bug process input_g (#1030) * Fixed the insight generator service (#1028) * Jiarima/added more testings (#1031) * Added another test Updated ReplaceHashesInTemplate function to return string instead of Template * Added third test * Merged * Reverted the workflow file to match with the one in hack/insights * Bugs fixes to hook insights up to ADS (#1033) * Bug fixes for hack insights (#1032) * Fixed the minColumn index bug in Data Transformation Fixed the template matching logic. * Adding changes from PR * Try to fix Workflow tests * Readd workflow tests * Fix template load location Co-authored-by: Aasim Khan <aasimkhan30@gmail.com> Co-authored-by: Nara <NaraVen@users.noreply.github.com> Co-authored-by: arslan9955 <53170027+arslan9955@users.noreply.github.com> Co-authored-by: Arslan Memon <armemon@microsoft.com> Co-authored-by: gadudhbh <68879970+gadudhbh@users.noreply.github.com> Co-authored-by: Jinjing Arima <jiarima@microsoft.com> Co-authored-by: jiarima <68882862+jiarima@users.noreply.github.com> Co-authored-by: Kim Santiago <31145923+kisantia@users.noreply.github.com> Co-authored-by: Udeesha Gautam <46980425+udeeshagautam@users.noreply.github.com> Co-authored-by: khoiph1 <khoiph@microsoft.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.