spatialmodel / inmap

InMAP reduced-form air quality model for fine particulate matter (PM2.5)
GNU General Public License v3.0
61 stars 42 forks source link

gobra added #30

Closed khoin closed 6 years ago

khoin commented 6 years ago

In this PR:

I've made cobra to work with gobra.

I think you will need to go get skratchdot/open-golang first before it runs properly. I also don't think the binary will reflect this change unless you re-compile it, so on my machine, I tested it with go run main.go in /inmap.

It should look something like this:

image

When the web interface is loaded, the model would run properly if you specify the config file. The output, is still dumped to the CLI:

image

... But text generated by cobra would be dumped to the web interface: image

About the code:

I saw that you intend to have the "HTTPPort" flag as a new flag for gobra. If you look into inmaputil/cmd.go at line 581, you'll see that I tried to not load the server if the HTTPPort flag is not specified. However, Cfg.GetString("HTTPPort") always return "8080" even when I run go run main.go --HTTPPort=0. I figured this is not important for now, so this PR goes through.

Let me know what you think.

khoin commented 6 years ago

@ctessum

khoin commented 6 years ago

Also, when you try to run steady on the web interface. You have to execute twice. I don't know why it is like that. I think when you run it the first time, it'll initialize the model, and the second time for the model to actually run.

ctessum commented 6 years ago

Awesome!

One issue is that InMAP uses dependency vendoring, which causes some problems here at least for me when I try to run it. To fix this, install the dep program and then run dep ensure from the main inmap directory. This will make some changes to the Gopkg.* files, which you should check into git, and will also download a bunch of files to the 'vendor' directory, which you should not check in.

I'm not sure why it needs to be run twice to make it work, but I think it is important to fix it so it works the first time. Beyond confusing users, my concern is that even when it works sometimes it might be running with a configuration different from the one most recently specified.

ctessum commented 6 years ago

Also, can you add your name to CONTRIBUTORS.md and add an entry to CHANGELOG.md?

ctessum commented 6 years ago

Great. Let me know if there is anything I can do to expedite the room key issue.

On Thu, Nov 30, 2017, 2:32 PM khoin notifications@github.com wrote:

@khoin commented on this pull request.

In inmap/index.html https://github.com/spatialmodel/inmap/pull/30#discussion_r153995033:

@@ -0,0 +1,804 @@ + +<!DOCTYPE html>

If I get the room key from CE frontdesk on Friday, ETA for all of these should be Friday afternoon (your Saturday), except for ctessum/gobra#11 https://github.com/ctessum/gobra/issues/11 and #30 (comment) https://github.com/spatialmodel/inmap/pull/30#discussion_r153989808 . These two will take probably take more time.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/spatialmodel/inmap/pull/30#discussion_r153995033, or mute the thread https://github.com/notifications/unsubscribe-auth/AF6AAU2qY24D5L_N59pwC5PkuJU4vwBkks5s7kv7gaJpZM4Qvh4a .

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.3%) to 85.033% when pulling 81ae2bce0d515e3fd34aaeb9e294aba5bbd96d0d on khoin:gobra into 70798a4459bf30f9b0a1bc7aa38a7d5cf1d96938 on spatialmodel:master.

khoin commented 6 years ago

I made some changes:

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.6%) to 84.66% when pulling 111ee1572641a5b738509940304132ab2aa9e970 on khoin:gobra into 70798a4459bf30f9b0a1bc7aa38a7d5cf1d96938 on spatialmodel:master.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.6%) to 84.66% when pulling 111ee1572641a5b738509940304132ab2aa9e970 on khoin:gobra into 70798a4459bf30f9b0a1bc7aa38a7d5cf1d96938 on spatialmodel:master.

khoin commented 6 years ago

I've merged the PR on gobra. I've also merged your PR on my inmap fork. I've squashed all the commits into one now.

Most issues mentioned in this PR should be resolved. Other issues more related to Gobra will be resolved in that repo.

Your turn.

ctessum commented 6 years ago

Great. Thanks!