OpenEnergyPlatform / ontology

Repository for the Open Energy Ontology (OEO)
Creative Commons Zero v1.0 Universal
111 stars 23 forks source link

Add task definition in separate module #1891 #1940

Closed madbkr closed 2 weeks ago

madbkr commented 1 month ago

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

Added classes:

'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

Added axioms:

Added Object Properties

Workflow checklist

Automation

Closes #

PR-Assignee

Reviewer

madbkr commented 1 month ago

@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?

stap-m commented 1 month ago

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 commented 1 month ago

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)

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 commented 1 month ago

@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.

stap-m commented 1 month ago

I have two general remarks:

  1. I am wondering for all these cases with the long labels, whether the definitions would read more smoothly if they started with "A" (indefinite article)). Currently, they start directly with the label, which is also ok in the Aristotelian style.
  2. Many/most of the action specification labels sound like processes, e.g. data acquisition and pre-processing. We usually try to avoid that. That could be done by adding the word "action" of "method". However, this would make the labels even more clumsy.

Any opinions @madbkr @LillyG901

madbkr commented 1 month ago

@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.

stap-m commented 1 month ago

@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.

madbkr commented 1 month ago

@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.

madbkr commented 3 weeks ago

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

stap-m commented 3 weeks ago

@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 commented 3 weeks ago

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

stap-m commented 3 weeks ago

@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.

madbkr commented 3 weeks ago

@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 commented 2 weeks ago

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