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.

Sign up for GitHub

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

Jump to bottom

fix: Type error in GCBM API causes (fatal) - main(306) - Value too large. error. #197

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

fix: Type error in GCBM API causes (fatal) - main(306) - Value too large. error. #197

wants to merge 14 commits into from

Conversation

Freeman-kuch
Copy link

Pull Request Template

Description

When /dynamic endpoint is visited during simulation, it raises Type error in GCBM API causes (fatal) - main(306) - Value too large. error with config files

Fixes #136

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

I tested locally using Postman and added the necessary config files in GCBM_New_Demo_Run. The simulation did run and didn't raise further errors.

fix

...

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings
  • I have tagged the reviewers in a comment below incase my pull request is ready for a review
  • I have signed the commit message to agree to Developer Certificate of Origin (DCO) (to certify that you wrote or otherwise have the right to submit your contribution to the project.) by adding "--signoff" to my git commit command.

aornugent
aornugent reviewed Oct 15, 2022
View reviewed changes
local/rest_api_gcbm/preprocess.py
@@ -153,15 +149,15 @@ def get_input_layers():
) as json_file:
dictionary["layer_type"] = "GridLayer"
dictionary["layer_data"] = "Int16"
dictionary["nodata"] = nodatam[1]
dictionary["nodata"] = 32767
Copy link
Contributor

Choose a reason for hiding this comment

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

@Namyalg @Janvi-Thakkar @SanjaySinghRajpoot @YashKandalkar - what do you think of this solution?

Copy link
Member

Choose a reason for hiding this comment

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

I think, as a temporary fix, we can probably go ahead with this

Copy link
Contributor

Choose a reason for hiding this comment

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

We might also need a comment explaining this temporary solution, so that other contributors know why the hard coded value is there.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Freeman-kuch - can you please add documentation to this patch? Using a hard coded number is not a good long term solution, but it will do for now.

Copy link
Author

Choose a reason for hiding this comment

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

@aornugent I'm not so sure i understand what you mean about adding documentation, can you help out ?

Copy link
Contributor

@aornugent aornugent Dec 20, 2022 •
edited

Choose a reason for hiding this comment

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

I mean leaving a comment in the code to explain your change. Without documentation it's hard for others to understand why this value is set.

Maybe something like:

# FIXME: hard coded nodata value is a temporary solution, 
but should instead come from the input data source.

Namyalg
Namyalg reviewed Oct 16, 2022
View reviewed changes
Copy link
Member

@Namyalg Namyalg left a comment

Choose a reason for hiding this comment

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

Yes, I think we can add a comment and add the link to the issue, so the problem becomes clear

HarshCasper
HarshCasper requested changes Oct 16, 2022
View reviewed changes
Copy link
Member

@HarshCasper HarshCasper left a comment

Choose a reason for hiding this comment

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

Please run black and flake8 on your code and fix all formatting/style issues.

@HarshCasper HarshCasper mentioned this pull request Oct 16, 2022
Closed
@Freeman-kuch
Copy link
Author

Freeman-kuch commented Oct 16, 2022 •
edited

Please run black and flake8 on your code and fix all formatting/style issues.

I did Run both and no issue was raised

khanjasir90 and others added 11 commits October 20, 2022 13:00
@khanjasir90 @Freeman-kuch
chore: moved get_config_templates, get_modules_cbm_config, get_pr… (m…
327b87b 
…oja-global#181)

* refactor: moved get_config_templates, get_modules_cbm_config, get_provider_config to preprocess.py file

Signed-off-by: khanjasir90 <khanmohdjasir@gmail.com>

* app.py and preprocess.py linted

Signed-off-by: khanjasir90 <khanmohdjasir@gmail.com>

Signed-off-by: khanjasir90 <khanmohdjasir@gmail.com>
Signed-off-by: Freeman <freemanoparaocha@gmail.com>
@SanjaySinghRajpoot @Freeman-kuch
feat: added Furo theme, GCBM dev docs and Restructured deployments do…
6930376 
…cs (moja-global#164)

* feat: added fluro and GCBM dev docs

Signed-off-by: SanjaySinghRajpoot <iamsanjayrajpoot@gmail.com>

* chores: furo added to requirements

Signed-off-by: SanjaySinghRajpoot <iamsanjayrajpoot@gmail.com>

* chores: restructed Deployment docs

Signed-off-by: SanjaySinghRajpoot <iamsanjayrajpoot@gmail.com>

Signed-off-by: SanjaySinghRajpoot <iamsanjayrajpoot@gmail.com>
Signed-off-by: Freeman <freemanoparaocha@gmail.com>
@SanjaySinghRajpoot @Freeman-kuch
feat: autoapi implemented to document APIs (moja-global#173)
206e060 
* feat: autoapi implemented

* chore: autoapi added to requirements

Signed-off-by: SanjaySinghRajpoot <iamsanjayrajpoot@gmail.com>

* CI fix

Signed-off-by: SanjaySinghRajpoot <iamsanjayrajpoot@gmail.com>

Signed-off-by: SanjaySinghRajpoot <iamsanjayrajpoot@gmail.com>
Signed-off-by: Freeman <freemanoparaocha@gmail.com>
@Crystalsage @Namyalg @Freeman-kuch
docs: Add responses to the GCBM docs (moja-global#184)
d37895d 
docs: Add expected response codes on sucess to the GCBM docs

Adds response codes to the GCBM curl documentation. Also changes
the format of the document to be more navigable.

Signed-off-by: Amit <amitkulkarni2028@gmail.com>

Signed-off-by: Amit <amitkulkarni2028@gmail.com>
Co-authored-by: Namya LG <53875297+Namyalg@users.noreply.github.com>
Signed-off-by: Freeman <freemanoparaocha@gmail.com>
@khanjasir90 @Namyalg @Freeman-kuch
feat: added endpoint to return .json for the input .tiff file (moja-g…
ba530db 
…lobal#185)

* feat: added endpoint to return .json for the input .tiff file

Signed-off-by: khanjasir90 <khanmohdjasir@gmail.com>

* feat: updated endpoint with specified changes and some constraints

Signed-off-by: khanjasir90 <khanmohdjasir@gmail.com>

* feat: formatted code with black

Signed-off-by: khanjasir90 <khanmohdjasir@gmail.com>

* updated minor typos

Signed-off-by: khanjasir90 <khanmohdjasir@gmail.com>

Signed-off-by: khanjasir90 <khanmohdjasir@gmail.com>
Co-authored-by: Namya LG <53875297+Namyalg@users.noreply.github.com>
Signed-off-by: Freeman <freemanoparaocha@gmail.com>
@SanjaySinghRajpoot @Freeman-kuch
Updated curl cmds for the GCBM dynamic endpoints (moja-global#194)
6232b30 
* feat: updated curl cmds

Signed-off-by: SanjaySinghRajpoot <iamsanjayrajpoot@gmail.com>

* db added and doc formated

Signed-off-by: SanjaySinghRajpoot <iamsanjayrajpoot@gmail.com>

* example text fixed

Signed-off-by: SanjaySinghRajpoot <iamsanjayrajpoot@gmail.com>

Signed-off-by: SanjaySinghRajpoot <iamsanjayrajpoot@gmail.com>
Signed-off-by: Freeman <freemanoparaocha@gmail.com>
@temitayopelumi @Freeman-kuch
chore:removed /rothc endpoint description from swagger documentation …
1e63bac 
…of rest api gcbm (moja-global#195)

Signed-off-by: olalekan temitayo <olalekantemitayo7@gmail.com>

Signed-off-by: olalekan temitayo <olalekantemitayo7@gmail.com>
Signed-off-by: Freeman <freemanoparaocha@gmail.com>
@Saurabh-Suchak @HarshCasper @Freeman-kuch
Improved Documentation for GCBM REST API (moja-global#144)
f8483b6 
* Update README.md

* Update README.md

* Update README.md

* Update README.md

* Update README.md

* Update README.md

* Update README.md

* changed the endpoint name

* Update app.py

* Update README.md

Co-authored-by: Harsh Mishra <erbeusgriffincasper@gmail.com>
Signed-off-by: Freeman <freemanoparaocha@gmail.com>
@Freeman-kuch
fixed the linting problem
0711668 
Signed-off-by: Freeman <freemanoparaocha@gmail.com>
@Freeman-kuch
Signed-off-by:Freeman <freemanoparaocha@gmail.com>
d6b8883 
Signed-off-by: Freeman <freemanoparaocha@gmail.com>
@temitayopelumi @Freeman-kuch
fix: add a check to see if simulation output exists (moja-global#199)
bb8daaa 
Signed-off-by: olalekan temitayo <olalekantemitayo7@gmail.com>

Signed-off-by: olalekan temitayo <olalekantemitayo7@gmail.com>
Signed-off-by: Freeman <freemanoparaocha@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aornugent aornugent aornugent left review comments

@Namyalg Namyalg Namyalg left review comments

@HarshCasper HarshCasper Awaiting requested review from HarshCasper

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Type error in GCBM API causes (fatal) - main(306) - Value too large. error.
9 participants
@Freeman-kuch @aornugent @HarshCasper @Namyalg @khanjasir90 @SanjaySinghRajpoot @Crystalsage @temitayopelumi @Saurabh-Suchak