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

add potential as parent class #1093 #1102

Merged
merged 15 commits into from
May 3, 2022
Merged

add potential as parent class #1093 #1102

merged 15 commits into from
May 3, 2022

Conversation

markus-rothkoetter
Copy link
Contributor

closes #1093

@markus-rothkoetter
feat(new-term): ✨ add class 'potential'
8006481 
add class as subclass of quantity value
add label + definition

TODO: add term-tracker
@markus-rothkoetter
feat(restructure): ✨ move children to 'potential'
13407d5 
move
- 'flow potential'
- 'stock potential'
into the new parent class 'potential'

TODO: add term-tracker
@github-actions github-actions bot added the oeo-physical changes the oeo-physical module label Apr 6, 2022
@markus-rothkoetter markus-rothkoetter added [A] new term Including new term(s) in the ontology [B] restructure Restructuring existing parts of the ontology labels Apr 6, 2022
@markus-rothkoetter
Copy link
Contributor Author

@l-emele, @stap-m
I'd be happy, if one of you could review my draft PR as well as commits.
Just to make sure that I didn't forget anything before eventually pull requesting.

I also have to remaining workflow questions:

Question 1:
Is it necessary to add the term-tracker information to the moved classes?
('flow potential' and 'stock potential')

The wiki/term-tracker-item states

If there are already term tracker annotations for this term, add your term track AT THE TOP of the annotation.
But I'm unsure if this applies to restructurings as well ...

Question 2:
Protégé also updated the timestamp in 'catalog-v001.xml'.
Shall these changes be staged to?
If yes, I'll add a commit.
(I read in the documentation that it's used as config for imports, so I was wondering if this is rather a local thing.)

@markus-rothkoetter markus-rothkoetter changed the title Feature 1093 add potential as parent class add potential as parent class #1093 Apr 6, 2022
@l-emele l-emele requested review from l-emele and stap-m April 6, 2022 08:07
@l-emele l-emele marked this pull request as ready for review April 6, 2022 08:07
l-emele
l-emele requested changes Apr 6, 2022
View reviewed changes
Copy link
Contributor

@l-emele l-emele left a comment

Choose a reason for hiding this comment

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

I added @stap-m and myself as reviewers and pressed the "Ready for review" button.

Question 1:
Is it necessary to add the term-tracker information to the moved classes?
('flow potential' and 'stock potential')

Yes, the term tracker needs to be updated. But more importantly, also the definitions need slight updates. As these are now subclasses of potential, the definition has to be now: A flow potential is a potential that ... and A stock potential is a potential that ... Also additions/deletions/changes have to be documented in CHANGELOG.md (simply naming the class labels and PR number in the respective categories of the "unreleased" section.

Please be aware, that the wiki entry did not reflect, what we really to regarding the term tracker items. I changed it just now. We add new entries at the bottom.

Question 2:
Protégé also updated the timestamp in 'catalog-v001.xml'.
Shall these changes be staged to?
If yes, I'll add a commit.
(I read in the documentation that it's used as config for imports, so I was wondering if this is rather a local thing.)

I am always unsure what to do with the catalog file. @jannahastings : Shall these changes be included or not?

@jannahastings
Copy link
Contributor

I am always unsure what to do with the catalog file. @jannahastings : Shall these changes be included or not?

The only time that changes to the catalog need to be committed are if we add new files, e.g. new modules, or change the way that the files are structured in directories. Otherwise, ignore it (perhaps add to your .gitignore?)

@markus-rothkoetter
docs: 📚️ add info about 'catalog.xml's
1a29ff8 
add a quick information concerning when to stage changes or not
@markus-rothkoetter
feat(def-update): ✨ adjust subclass definition
0dbcaf8 
adjust
- 'flow potential'
- 'stock potential'
to base their definition on the new parent class 'potential'
@markus-rothkoetter
feat(term-tracker): ✨ append updates to subclass term tracker
09b79df 
affected subclasses:
- 'stock potential'
- 'flow potential'
@markus-rothkoetter
docs(changelog): 📚️ add update regarding PR #1102
a212f67
@markus-rothkoetter
Copy link
Contributor Author

Yes, the term tracker needs to be updated.

I updated it accordingly.
Nevertheless, I'm asking myself why the changes shall be appended at the bottom.

Isn't it more reasonable to have the changes at the top in order to see the most recent ones at first?

But more importantly, also the definitions need slight updates. As these are now subclasses of potential, the definition has to be now: A flow potential is a potential that ... and A stock potential is a potential that ...

Done. (Hope so ...)

I adjusted it, so that all redundant information covered by the parent class are removed e.g.:

A flow potential is a potential that describes the input or output of a process per time unit.

... is the updated version of ...

A flow potential is a quantity value that describes 
the upper limit of an input or output of a process in a spatial region of reference per unit time.

... based on ...

A potential is a quantity value that describes some upper limit in a spatial region of reference.

Is this how it is supposed to be?

Also additions/deletions/changes have to be documented in CHANGELOG.md (simply naming the class labels and PR number in the respective categories of the "unreleased" section.

Ah, this was a obvious one that I overlooked.

@markus-rothkoetter
Copy link
Contributor Author

I am always unsure what to do with the catalog file. @jannahastings : Shall these changes be included or not?

The only time that changes to the catalog need to be committed are if we add new files, e.g. new modules, or change the way that the files are structured in directories. Otherwise, ignore it (perhaps add to your .gitignore?)

I also added a "quick tip" to the CONTRIBUTING.md stating this.
In case this is no good idea, we can simply drop the specific commit.

stap-m
stap-m requested changes Apr 7, 2022
View reviewed changes
Copy link
Contributor

@stap-m stap-m left a comment

Choose a reason for hiding this comment

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

The rest looks fine!

@markus-rothkoetter
Copy link
Contributor Author

I'd put it even shorter...

Perfect. I was unsure how much simpler it shall be.

@markus-rothkoetter
Copy link
Contributor Author

@l-emele Could you re-review this PR?
(I'm not sure if my re-request worked)

@markus-rothkoetter
Copy link
Contributor Author

@l-emele
The proposed adjustments unfortunately break the oeo-physical import.

I verified the online failure with an offline test in Protégé – same result.
As I did my last adjustment in the GitHub WebEditor, I also crossed checked, if the error persists when using a offline editor – same result.

All my previous changes were made in Protégé, so the full iri was inserted by the programm.
Is there a configuration option, that I'm not aware of, to prevent this?

Can you indicate a solution how to incorporate these changes without breaking the oeo-physical-file?

@stap-m stap-m mentioned this pull request May 2, 2022
Closed
5 tasks
@l-emele
Copy link
Contributor

l-emele commented May 3, 2022

Hm, not good if this brakes things. I just looked through the whole file. In most cases, there is just the ID (like OEO_0000abcd), but in few cases there are there are the full IRIs (like <http://openenergy-platform.org/ontology/oeo/oeo-physical/OEO_00010239>). @jannahastings : Any idea, why this is?

If we don't find a quick solution, I suggest that you ignore my last comment and revert to the working version with the IRIs.

@markus-rothkoetter
Copy link
Contributor Author

I think this relates to the "specified IRI" from Protégé.
While working on #1112 I noticed that the active IRI was inserted.
I'll come back to this and test if this is the case after finishing #1112.

@markus-rothkoetter markus-rothkoetter merged commit b7ac449 into dev May 3, 2022
@markus-rothkoetter markus-rothkoetter deleted the feature-1093-add-potential-as-parent-class branch May 3, 2022 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@l-emele l-emele l-emele approved these changes

@stap-m stap-m stap-m approved these changes

Assignees
No one assigned
Labels
[A] new term Including new term(s) in the ontology [B] restructure Restructuring existing parts of the ontology oeo-physical changes the oeo-physical module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add parent class potential
4 participants
@markus-rothkoetter @jannahastings @l-emele @stap-m