-
Notifications
You must be signed in to change notification settings - Fork 81
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
Feature: GeoMetaMaker integration #1753
Feature: GeoMetaMaker integration #1753
Conversation
@@ -505,29 +504,5 @@ def test_model_specs_serialize(self): | |||
f'{error}') | |||
|
|||
|
|||
class SpecUtilsTests(unittest.TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this set of tests to test_spec_utils
* } | ||
* license: { | ||
* title: string | ||
* url: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like in other places url
is actually called path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, not ideal. geometamaker
is following the DataPackage schema, which uses path
. But url
is more meaningful so that's why the Workbench components are using it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fantastic! I was also able to try this out in the workbench and CLI and everything worked as expected (including that the metadata was recreated each time the model was run even with the same inputs/params, which is great).
in the output workspace after running a model. | ||
</p> | ||
<p> | ||
Open a YAML file in a text editor to read the metadata. It includes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worth mentioning in this info panel that users can also edit the metadata in a text editor (and give an example of why they might wish to do so)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, for now I only changed the sentence to
Open a YAML file in a text editor to read the metadata and even add to it.
Maybe we'll get more feedback at some point and possibly have some other documentation to point to. For now I don't want to distract too much from just giving people the context they need to fill out the form in the Workbench.
@claire-simpson and @emilyanndavis , I added a small addition to write |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool, Dave! Please don't be alarmed by the number of comments. Pretty minor stuff for the most part!
@@ -5,6 +5,7 @@ import Modal from 'react-bootstrap/Modal'; | |||
import { MdClose } from 'react-icons/md'; | |||
import { useTranslation } from 'react-i18next'; | |||
|
|||
import { openLinkInBrowser } from '../../utils'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call moving this to utils; thanks!
the bands in a raster, and other useful information. | ||
</p> | ||
<p> | ||
Some properties of the metadata are configureable here. You may |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
configureable
=> configurable
onClick={openLinkInBrowser} | ||
> | ||
https://github.com/natcap/geometamaker | ||
</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we give this link user-friendly link text instead of using the raw URL? Maybe something like this?
<p>
InVEST uses
<a
href="https://github.com/natcap/geometamaker"
onClick={openLinkInBrowser}
>GeoMetaMaker</a>
to generate metadata.
</p>
... or like this?
<p>
InVEST uses GeoMetaMaker to generate metadata. Learn more about
<a
href="https://github.com/natcap/geometamaker"
onClick={openLinkInBrowser}
>GeoMetaMaker on Github</a>.
</p>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the second one. One thing I like about the bare URL is that it lets you know where you're headed. But this second option is a nice improvement on that.
|
||
function AboutMetadataDiv() { | ||
return ( | ||
<div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming we'll eventually have translations of this copy into other languages. Is it worth adding support for that now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks for the suggestion.
? <AboutMetadataDiv /> | ||
: ( | ||
<Form onSubmit={handleSubmit}> | ||
<fieldset> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome use of fieldsets & legends! 🌟
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I learned something new!
} | ||
|
||
#metadata-form fieldset { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like an empty ruleset that can be removed.
src/natcap/invest/spec_utils.py
Outdated
resource.set_lineage(lineage_statement) | ||
# a pre-existing metadata doc could have keywords | ||
words = resource.get_keywords() | ||
resource.set_keywords(words + keywords_list) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any chance any of the words
could also be in keywords_list
? Would it be worth checking for duplicates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After testing this locally, and looking at the code again, I can confirm the answer to the first question is yes! And I would recommend checking for and removing duplicates. I ran the carbon model a few times, and I ended up with the keywords list carbon, InVEST, carbon, InVEST, carbon, InVEST
. That could add up quickly. 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this!
tests/test_spec_utils.py
Outdated
spec_utils.format_unit({}) | ||
|
||
|
||
class TestDescribeArgFomSpec(unittest.TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be TestDescribeArgFromSpec
?
eventKey="0" | ||
className="mr-2 w-50" | ||
> | ||
<span><u>{t('Configure Metadata')}</u></span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was surprised to see this text underlined: it had the effect of making me think it was a link embedded in a button. Would you be OK with removing the underline?
I also have a couple other ideas to help make it look more obviously like an accordion; let me know if you want my help, either now or in a follow-up PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We talked about this on Slack. I implemented some of your ideas.
if (profile.license) { | ||
setLicenseTitle(profile.license.title); | ||
setLicenseURL(profile.license.path); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this out locally and was seeing a Cannot read properties of undefined (reading 'contact')
error. The root cause was that I'd forgotten to install invest
, so the get profile endpoint was returning a 404
. Once I installed invest, the errors went away, so in theory this shouldn't happen IRL, but just in case, it wouldn't hurt to add a null check on profile
to these if
statements, i.e., if (profile && profile.contact)
and if (profile && profile.license)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thanks, Dave!
The
invest run
CLI now usesgeometamaker
to generate metadata documents for all files listed in a model'sOUTPUT_SPEC
.The Workbench includes a new feature to allow users to configure
geometamaker
with a "user profile".There's a corresponding User's Guide PR which we could merge first: natcap/invest.users-guide#157
Fixes #1662
Checklist