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

Merging CISLAM Experimental model (no Test Suite available) in JGrassTools #8

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

mcfoi
Copy link
Contributor

@mcfoi mcfoi commented Oct 22, 2014

The wrapper of the main module is working correctly. All Oms sub-modules are fine. Wrappers for sub-modules might not be fully functional.

@mcfoi mcfoi changed the title Merging CISLAM Experimental model (not Test Suite available) in JGrassTools Merging CISLAM Experimental model (no Test Suite available) in JGrassTools Oct 22, 2014
@In
public double pAlfaVanGen = 6.5;
*/
@Description(OMSCISLAM_inNVanGen_DESCRIPTION)
Copy link
Member

Choose a reason for hiding this comment

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

Hi, can I ask you to remove commented variables? If they are not needed, please just remove them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot this mail: now OmsCislam should be free from dead/commented code.

On 3 November 2014 03:50, Andrea Antonello [email protected] wrote:

In
hortonmachine/src/main/java/org/jgrasstools/hortonmachine/modules/hydrogeomorphology/cislam/OmsCislam.java:

  • */
  • // #############################################
  • // van Genuchten parameters
  • // #############################################
  • @description(OMSCISLAM_inAlfaVanGen_DESCRIPTION)
  • @Unit("1/m")
  • @ui(JGTConstants.FILEIN_UI_HINT)
  • @in
  • public GridCoverage2D inAlfaVanGen = null;
  • /*
  • @description(OMSCISLAM_pAlfaVanGen_DESCRIPTION)
  • @in
  • public double pAlfaVanGen = 6.5;
  • */
  • @description(OMSCISLAM_inNVanGen_DESCRIPTION)

Hi, can I ask you to remove commented variables? If they are not needed,
please just remove them.


Reply to this email directly or view it on GitHub
https://github.com/moovida/jgrasstools/pull/8/files#r19717772.

@moovida
Copy link
Member

moovida commented Nov 3, 2014

@mcfoi , I gave a first quick look at everything and commented inline as you can see.

One thing that is wrong more or less everywhere, is the use of:
@ui(JGTConstants.FILEIN_UI_HINT) for in and out.

In the Oms* modules, there is no need for that, since they are not loaded in the GUI. And environments that want to generate one for GridCoverages and FeatureCollections do not need that. This UI annotation has been introduced only when using file paths, since in that case it is string and we never know if it is a path or something different. So check the inputs and outputs. Remove them from the Oms* classes and add it to the modules classes.

@mcfoi mcfoi closed this Nov 4, 2014
@mcfoi
Copy link
Contributor Author

mcfoi commented Nov 4, 2014

I fixed everything else beside the issued I commented back.
Also the problem with @ui(JGTConstants.FILEIN_UI_HINT) in Oms* modules was
fixed.

Marco

On 3 November 2014 04:16, Andrea Antonello [email protected] wrote:

@mcfoi https://github.com/mcfoi , I gave a first quick look at
everything and commented inline as you can see.

One thing that is wrong more or less everywhere, is the use of:
@ui https://github.com/UI(JGTConstants.FILEIN_UI_HINT) for in and out.

In the Oms* modules, there is no need for that, since they are not loaded
in the GUI. And environments that want to generate one for GridCoverages
and FeatureCollections do not need that. This UI annotation has been
introduced only when using file paths, since in that case it is string and
we never know if it is a path or something different. So check the inputs
and outputs. Remove them from the Oms* classes and add it to the modules
classes.


Reply to this email directly or view it on GitHub
#8 (comment).

@mcfoi mcfoi reopened this Nov 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants