Skip to content
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

Fix Meta modules and add inputs to ndvi-colormap #1432

Merged
merged 7 commits into from
Jan 8, 2020

Conversation

rishabhshuklax
Copy link
Member

@rishabhshuklax rishabhshuklax commented Jan 6, 2020

Fixes #1239 ,Fixes #1418 , Fixes #1431

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with npm test
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • ask @publiclab/is-reviewers for help, in a comment below
  • Insert-step functionality is working correct as expected.

Below is the working GIFS
meta1

meta2

@rishabhshuklax rishabhshuklax changed the title Fix Meta modules and inputs to ndvi-colormap Fix Meta modules and add inputs to ndvi-colormap Jan 6, 2020
@rishabhshuklax
Copy link
Member Author

@publiclab/is-reviewers @jywarren @keshav234156 please review this again, In my last PR whole bug was not resolved.

@codecov
Copy link

codecov bot commented Jan 6, 2020

Codecov Report

Merging #1432 into main will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #1432   +/-   ##
=====================================
  Coverage   66.4%   66.4%           
=====================================
  Files        125     125           
  Lines       2581    2581           
  Branches     410     410           
=====================================
  Hits        1714    1714           
  Misses       867     867
Impacted Files Coverage Δ
src/util/createMetaModule.js 100% <100%> (ø) ⬆️

Copy link
Member

@harshkhandeparkar harshkhandeparkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -1,6 +1,24 @@
{
"name": "ndvi-colormap",
"description": "Sequentially Applies NDVI and Colormap steps",
"inputs": {},
"inputs": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow. That's a big fix. Very important.

// use this to set defaults for internal steps
// and to expose internal settings as external meta-module parameters;
// it must return a steps object
var steps = mapFunction(options);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tech4GT can you check this change of scope? Thank you!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linking to explanation: #1239 (comment)

@jywarren jywarren requested a review from tech4GT January 7, 2020 20:48
@jywarren
Copy link
Member

jywarren commented Jan 7, 2020

This is looking great; i asked @tech4GT to review as he most recently worked on these files. Would it be possible to confirm this change with a test? https://github.com/publiclab/image-sequencer/blob/main/test/core/modules/colormap.js

@keshav234156
Copy link
Member

@jywarren after #1239 the problem that still remained was that when option where changes is still processed the image with original option. This thing we are currently not testing that why they always passed. To test this we need to change https://github.com/publiclab/image-sequencer/blob/main/test/core/templates/module-test.js and all test modules using it .It would be a big change

@rishabhshuklax
Copy link
Member Author

Yes, this is the reason why the current tests weren't able to detect it and we can make those changes in a new PR and I will be happy to make these changes but lets merge this one for now.

@jywarren
Copy link
Member

jywarren commented Jan 8, 2020

OK, that sounds good. Just checking! Thank you!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants