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

Add task definition in separate module #1891 #1940

Merged
merged 53 commits into from
Nov 7, 2024

Conversation

madbkr
Copy link
Contributor

@madbkr madbkr commented Oct 7, 2024

Summary of the discussion

In an effort to add energy-related task definitions I added a separate module called oeo-tasks and tried to implement the needed classes as proposed in #1891. The module is not yet imported by the oeo. There are still some questions open as seen in the issue.

Type of change (CHANGELOG.md)

Add

  • new file oeo-tasks.omn for all of the changes proposed. This would be a separate module to the current oeo.

Added classes:

  • All the subtasks of a task as a subclasses of action specification. For better readability they get their own parent class. Labels were shortened.

'action specification for wind farm development area'
- 'determine development area'
- 'determine geographical potential'
- 'improve spatial resolution'
- 'prepare exclusion criteria datasets'
- 'run multi-criteria decision analysis'
- select exclusion criteria'

'action specification for for the technical wind potential'
- 'account for wake losses'
- 'calculate technical potential'
- 'determine annual energy yield'
- 'determine capacity density'
- 'determine capacity factor'
- 'place turbines within area'
- 'select appropriate turbine types'

'action specification for techno-economic potential'
- 'calculate economic potential'
- 'calculate levelised cost of energy'
- 'determine capital expenditures'
- 'determine discount and interest rate'
- 'determine operational expenditures'
- 'determine wind turbine lifetime'

'action specification for the feasible potential'
- 'apply constraints for assessment of potential'
- 'calculate feasible potential'

'action specification for wind characteristics'
- air density adjustment
- data preparation
- wind power density calculation
- wind speed calculation
- wind speed frequency analysis
- wind variability identification

  • Headline of the task as subclass of objective specification

    • determined feasible potential
    • 'determined techno-economic potential'
    • determined wind characteristics'
    • 'determined wind farm area'
    • 'determined wind potential'
  • Headline of the task + method as a subclass of methodology

    • feasible potential determination method
    • 'techno-economic potential determination method'
    • 'wind characteristics determination method'
    • 'wind farm area determination method'
    • 'wind potential determination method'
  • As a subclass of process

    • feasible potential determination process
    • 'techno-economic potential determination process'
    • 'wind characteristics determination process'
    • 'wind farm area determination process'
    • 'wind potential determination process'

Added axioms:

  • For wind characteristics determination method

    • 'has part' some 'determined wind characteristics'
  • For 'wind farm area determination process'

    • 'has part' some 'determined wind farm area'
  • For 'feasible potential determination method'

    • 'has part' some 'determined feasible potential'
  • For 'wind potential determination method'

    • 'has part' some 'determined wind potential'
  • For ' techno-economic potential determination method'

    • 'has part' some 'determined techno-economic potential'
  • For wind characteristics determination process

    • 'structured by' some 'wind characteristics determination method'
  • For 'wind farm area determination process'

    • 'structured by' some 'wind farm area determination method'
  • For 'feasible potential determination process'

    • 'structured by' some 'feasible potential determination process'
  • For 'wind potential determination process'

    • 'structured by' some 'wind potential determination method'
  • For 'techno-economic potential determination process'

    • 'structured by' some 'techno-economic potential determination method'

Added Object Properties

  • As a subclass of has participant
    • structured by

Workflow checklist

Automation

Closes #

PR-Assignee

Reviewer

  • 🐙 Follow the Reviewer Guide
  • 🐙 Provided feedback and show sufficient appreciation for the work done

@madbkr
Copy link
Contributor Author

madbkr commented Oct 7, 2024

@stap-m
Could you please let me know if this is what you had in mind? If not I'd like to correct my mistakes before I implement the other tasks.

Also it seems like I made a mistake and branched from my last feature (the small correction in the PR template) instead of from dev. I could use git revert but am unsure if that will cause even more problems when merging later. Any advice?

@madbkr madbkr self-assigned this Oct 7, 2024
@stap-m
Copy link
Contributor

stap-m commented Oct 8, 2024

Also it seems like I made a mistake and branched from my last feature (the small correction in the PR template) instead of from dev. I could use git revert but am unsure if that will cause even more problems when merging later. Any advice?

No worries. You can merge dev into this branch. This will probably cause merge conflicts, too, though.

@stap-m
Copy link
Contributor

stap-m commented Oct 8, 2024

Could you please let me know if this is what you had in mind? If not I'd like to correct my mistakes before I implement the other tasks

Yes, it goes into the right direction. But lets focus on the first tasks for now. I have some general remarks:

In general, especially for such a complex changes, please document your changes in the PR header a more detailed, such that I can directly see what you did (wanted to do)

  • which new class is of which kind, group the different types, ...
  • adding a new file(!)
  • complex axioms
  • ...

I'd recommend to discuss definitions and axioms in the issue first.

For the definitions, please use the Aristotelian schema "A xxx is a yyy that zzz". E.g. "An account for air density change is a ... ".

I not yet sure whether we actually need the relations that you added, they were rather for illustrating the complex model. That's why I used dashed lines. Sorry for not mentioning that.

@madbkr
Copy link
Contributor Author

madbkr commented Oct 8, 2024

@stap-m
Thank you for your answer.

In general, especially for such a complex changes, please document your changes in the PR header a more detailed, such that I can directly see what you did (wanted to do)

I was going to do that before converting the draft to a proper pull request. But now I know a lot more about what to write, so thank you.

I'd recommend to discuss definitions and axioms in the issue first.

Alright, then I'll move all the stuff I did there and wait for feedback.

For the definitions, please use the Aristotelian schema "A xxx is a yyy that zzz". E.g. "An account for air density change is a ... ".

I did try to do that. I really did. I'll go over them again.

I not yet sure whether we actually need the relations that you added, they were rather for illustrating the complex model. That's why I used dashed lines. Sorry for not mentioning that.

I thought they made everything more understandable but I am ready to remove them. I will move this question to the issue.

@madbkr madbkr changed the title Add task definition in seperate module #1891 Add task definition in separate module #1891 Oct 11, 2024
@stap-m
Copy link
Contributor

stap-m commented Oct 22, 2024

@stap-m I don't think the lables necessarily sound like processes but their length is very uncomfortable. It sounds more like a sentence than a name. If I had not used the given names I probably would have used a summarizing name for a task. For example Data preparation and included in the definition that this is about acquisition and pre-processing.

I like the idea. If you can find more meaningful labels that summarize the content, we could add either the shorend or the long ones as alternative labels.
EDIT: Please propose them in the issue first before committing.

src/ontology/edits/oeo-tasks.omn Outdated Show resolved Hide resolved
src/ontology/edits/oeo-tasks.omn Outdated Show resolved Hide resolved
src/ontology/edits/oeo-tasks.omn Outdated Show resolved Hide resolved
OEO_00020426 "Add:
issue: https://github.com/OpenEnergyPlatform/ontology/issues/1891
pull request: https://github.com/OpenEnergyPlatform/ontology/pull/1940",
<http://purl.obolibrary.org/obo/IAO_0000115> "Determine the theoretical potential / WPD is an action specification that describes how Wind Power Density (WPD) is calculated to estimate the theoretical wind power potential based on wind speed and air density.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<http://purl.obolibrary.org/obo/IAO_0000115> "Determine the theoretical potential / WPD is an action specification that describes how Wind Power Density (WPD) is calculated to estimate the theoretical wind power potential based on wind speed and air density.",
<http://purl.obolibrary.org/obo/IAO_0000115> "Determine the theoretical wind power potential / wind power density is an action specification that describes how wind power density (WPD) is calculated to estimate the theoretical wind power potential based on wind speed and air density.",

Please add to to do list in Issue:
We have something like theoretical potential and maybe also wind power density in the OEO. We should add axioms in a later step (OEO_00140063).

OEO_00020426 "Add:
issue: https://github.com/OpenEnergyPlatform/ontology/issues/1891
pull request: https://github.com/OpenEnergyPlatform/ontology/pull/1940",
<http://purl.obolibrary.org/obo/IAO_0000115> "Determine MCDA approach for soft exclusion criteria is an action specification that describes how a Multi-Criteria Decision Analysis (MCDA) method is applied to areas where exclusions are not absolute, allowing for weighted decision-making",
Copy link
Contributor

Choose a reason for hiding this comment

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

Add MCDA as separate concept?

@madbkr
Copy link
Contributor Author

madbkr commented Oct 22, 2024

@stap-m I don't think the lables necessarily sound like processes but their length is very uncomfortable. It sounds more like a sentence than a name. If I had not used the given names I probably would have used a summarizing name for a task. For example Data preparation and included in the definition that this is about acquisition and pre-processing.

I like the idea. If you can find more meaningful labels that summarize the content, we could add either the shorend or the long ones as alternative labels. EDIT: Please propose them in the issue first before committing.

Alright, I will try to come up with better lables for all of the tasks and propose adjusted definitions.

CHANGELOG.md Outdated Show resolved Hide resolved
src/ontology/edits/oeo-tasks.omn Outdated Show resolved Hide resolved
OEO_00020426 "Add:
issue: https://github.com/OpenEnergyPlatform/ontology/issues/1891
pull request: https://github.com/OpenEnergyPlatform/ontology/pull/1940",
<http://purl.obolibrary.org/obo/IAO_0000115> "Determined wind characteristics of a region is an object specification that defines the goal of a methodology as a state of understanding the specific wind characteristics of a predefined region.",
Copy link
Contributor

Choose a reason for hiding this comment

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

I might have missed the explanation of this in the issue: But what is the wind characteristic of a region? It might be a valid term on its own, but I feel like it could be explained / listed what attributes are subject to this: speed, ...?

Copy link
Contributor Author

@madbkr madbkr Oct 28, 2024

Choose a reason for hiding this comment

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

So far all of this has been more about the tasks that are part of the wind characteristics determination process. But I agree - it is a bit strange that we have no term that really explain many of the actual content of the tasks. Like what a wind characteristic is. Maybe we should add a few terms for that? Or at least expand the definitions?
@stap-m
I know we already pointed out some like MCDA but maybe those are not enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

See also #1891 (comment) 😺 I think we need input from the domain experts here

src/ontology/edits/oeo-tasks.omn Outdated Show resolved Hide resolved
src/ontology/edits/oeo-tasks.omn Outdated Show resolved Hide resolved
src/ontology/edits/oeo-tasks.omn Outdated Show resolved Hide resolved
OEO_00020426 "Add:
issue: https://github.com/OpenEnergyPlatform/ontology/issues/1891
pull request: https://github.com/OpenEnergyPlatform/ontology/pull/1940",
<http://purl.obolibrary.org/obo/IAO_0000115> "Action specification for wind characteristics is an action specification that deals with wind characteristics",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here regarding the unambiguous term of "wind characteristic".

src/ontology/edits/oeo-tasks.omn Outdated Show resolved Hide resolved
src/ontology/edits/oeo-tasks.omn Outdated Show resolved Hide resolved
src/ontology/edits/oeo-tasks.omn Outdated Show resolved Hide resolved
@madbkr
Copy link
Contributor Author

madbkr commented Oct 28, 2024

@ColinHDev
Thank you for your suggestions, I fixed them.

@stap-m
Copy link
Contributor

stap-m commented Oct 28, 2024

@madbkr I'll review again. I'd suggest that all open questions should be part of a second PR. Could you include them into the todo list please?

@madbkr
Copy link
Contributor Author

madbkr commented Oct 28, 2024

@stap-m
I'll update the lables now. Please wait with the review until the next push.

@stap-m
Copy link
Contributor

stap-m commented Oct 30, 2024

@madbkr please check my following suggestions. Did you already update the changelog after changing the labels?

I'll marked some more stuff as todo for the next PR. I prefer to review that separatly. The addition for wind characteristics seems to be still missing, lets also do that in the next PR. Please update the todo list in the issue.

src/ontology/edits/oeo-tasks.omn Outdated Show resolved Hide resolved
src/ontology/edits/oeo-tasks.omn Outdated Show resolved Hide resolved
src/ontology/edits/oeo-tasks.omn Outdated Show resolved Hide resolved
src/ontology/edits/oeo-tasks.omn Outdated Show resolved Hide resolved
src/ontology/edits/oeo-tasks.omn Outdated Show resolved Hide resolved
src/ontology/edits/oeo-tasks.omn Outdated Show resolved Hide resolved
src/ontology/edits/oeo-tasks.omn Outdated Show resolved Hide resolved
@madbkr
Copy link
Contributor Author

madbkr commented Oct 30, 2024

@stap-m
Thank you for all your suggested changes, I added the To Dos and commited the changes.

The changelog is also updated but I have to change the labels here in the PR still. I will do that right away.

@madbkr
Copy link
Contributor Author

madbkr commented Nov 6, 2024

@stap-m
I assume I can merge this now? You only told me to wait for your approval?

@madbkr madbkr merged commit 8770541 into dev Nov 7, 2024
3 checks passed
@madbkr madbkr deleted the feature-1891-add-task-definition-in-seperate-module branch November 7, 2024 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants