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

Should "results_to_objects" be the default post-processing? #33

Closed
uvchik opened this Issue Dec 16, 2015 · 24 comments

Comments

Projects
None yet
3 participants
@uvchik
Copy link
Member

uvchik commented Dec 16, 2015

At the moment it works like this:

my_energysystem.optimize()
pp.results_to_objects(my_energysystem.optimization_model)

Is it possible to write the results to the object by default and use an optional parameter to prevent it from doing so.

And if we write the results to the object, do we still need the 'optimization_model' attribute. Is it possible to remove it by an optional parameter or does it still holds essential informations?

I would like to dump the EnergySystem object with the results but it does not work if the 'optimization_model' attribute is still there.

@gnn

This comment has been minimized.

Copy link
Member

gnn commented Dec 16, 2015

The answer would be a wholehearted "maybe".
I agree that there should be a way of getting at the optimization results by default without going through the extra step of calling results_to_objects. But I disagree that calling this function immediately should be the default behaviour. The main reason for this is, that there was (and still is, but it's nearly ready for a first release) ongoing development on a standard way of returning optimization results, mainly happening on the features/solph-development branch.Have a look the OptimizationModel#results() method. @uvchik was kind enough to point out to me that saving this object in the EnergySystem would be a good idea, which I did.

The results "object" (currently just a special dictionary) is finalized so one can use without having to fear further breaking changes (modulo fixing broken features and adding missing ones).
There are some planned additional features though, but those should be backwards compatible and integrate well with the existing ones.

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Dec 16, 2015

So far i used the the results_to_objects function and found a way to get all flows into and out of a bus.

bus2 = [obj for obj in my_energysystem.entities if obj.uid == ('uid of the bus')][0]

print("Inputs:")
for inp in bus2.inputs:
    print(inp.uid)
    print(inp.results['out'][bus2.uid][0:3])

print("Outputs:")
for out in bus2.outputs:
    print(out.uid)
    print(out.results['in'][bus2.uid][0:3])
  1. How can I do this with the result dictionary?
  2. Do we still need the optimization_model attribute?
  3. Aren't there any meta results such as solver time? Where can we find these results? In a result attribute of the simulation object (my_energysystem.simulation.results['solver_time'])?
@gnn

This comment has been minimized.

Copy link
Member

gnn commented Dec 16, 2015

Wow. I just found out that I can edit other ppls comments... weird.

  1. Yell at me if that doesn't work:

    bus2 = [obj for obj in my_energysystem.entities
            if obj.uid == ('uid of the bus')][0]
           # sidenote: this reminds me that I still have an unposted
           #           comment/answer to #17 in my head
    
    # es is the EnergySystem
    print("Inputs:")
    for inp in bus2.inputs:
        print(inp.uid)
        print(es.results[inp][bus2][0:3])
    
    print("Outputs:")
    for out in bus2.outputs:
        print(out.uid)
        print(es.results[bus2][out][0:3])
    
    # I'm assuming that the `[0:3]` means that you only want the first 3 values
    # of the timeseries.
  2. No, I'm going to chuck out that one. Watch out for the next commit.

  3. Technically those are not really optimisation results but side effects,
    because they might differ between runs, even if the inputs of the
    optimisation problem are the same. As such I would put them somewhere on
    the optimisation model.

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Dec 16, 2015

  1. Yell! Yes [0:3] keeps the output readable. An answer to #17 would be great 😄

    Traceback (most recent call last):
    File "/home/uwe/rli-lokal/git_home/oemof_base/examples/development_examples/two_regions_example_b.py", line 28, in <module>
      print(TwoRegExample.results[inp][bus2][0:3])
    KeyError: <oemof.core.network.entities.components.transports.Simple object at 0x7facffdd6b00>

    Maybe you forgot to write the transport results to the results dictionary?

  2. Looking forward.

  3. We can discuss this later. So far I do not miss any results. If there are any, we'll find a place to store them.

@gnn

This comment has been minimized.

Copy link
Member

gnn commented Dec 16, 2015

That is seriously weird. I was about to ask whether it worked before, but I guess that's a given.
I looked into results_to_objects and the transport results aren't written to any object there either, so I don't really have an idea where those are coming from.
Are they true optimization results or are those values known beforehand? Doesn't change the fact that they should be in there, but would help me know where to look.
Other than that, I probably have to ask @simonhilpert where those values come from.

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Dec 16, 2015

Sorry, it worked before, because I fixed it in my feature-branch (see: 6832be4) planing to fix it in the dev-branch after testing.

@gnn

This comment has been minimized.

Copy link
Member

gnn commented Dec 16, 2015

Cool. Getting to know the codebase a little bit better.
Fix pushed.

@simnh

This comment has been minimized.

Copy link
Member

simnh commented Dec 16, 2015

I must confess that I do not get it but I ll try to give a statement anyway:

The results_to_objects function was just a basic sketch, and transport results aren't included until now.
I think the new standard way will be the new results dictionary beeing developed by @gnn.
As it will be a method inside OptimizationModel() there will be no need of course to pass energysystem.optimization_model but rather just accessing via:

energysystem.optimization_model.results

#or
energysystem.results    #which will then do the above command...

if it's still emtpy, fill the results attribute...

I think this would be a good way.

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Dec 16, 2015

@gnn Thank you, works now 😄

Will you close this issue after (2. remove optimization_model attribute) is solved?

@gnn

This comment has been minimized.

Copy link
Member

gnn commented Dec 16, 2015

Did that with commit 6258d23. Didn't know whether there was anything more attached to it.
Otherwise I would have given "close by commit message" another try.

@gnn gnn closed this Dec 16, 2015

@gnn

This comment has been minimized.

Copy link
Member

gnn commented Dec 16, 2015

Reopened, as I want to leave this open until I made an appropriate remark in the whatsnew section.

@gnn gnn reopened this Dec 16, 2015

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Dec 16, 2015

It will be closed on Tuesday, when I merge everything into the master branch.

https://github.com/blog/1386-closing-issues-via-commit-messages

Did that with commit 6258d23. Didn't know whether there was anything more attached to it.

Didn't see it. Thanks 🏆

@simnh

This comment has been minimized.

Copy link
Member

simnh commented Dec 16, 2015

I think it would be good to add the optimization model as well to the energysystem, as there are a lot of informations inside the om which users can obtain. @gnn last commit 6258d23 deletes the optimization model as an attribute from energysystem object.
Maybe at least an option would be nice, like "keep model" or similar...

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Dec 17, 2015

@simonhilpert wrote:

I think it would be good to add the optimization model as well to the energysystem, as there are a lot of informations inside the om which users can obtain.

This was question 3. from above. What informations are inside the om? What about my suggestion to store these information anywhere (e.g. in the simulation object) and then remove the om.

Of course it should be possible to keep the om, but @gnn told me that it is still possible to keep the om without using an additional option. @gnn could you explain it?

@simnh

This comment has been minimized.

Copy link
Member

simnh commented Dec 17, 2015

Well first of all all reduced costs and shadowprices (dual variables). These values can of course be moved into the results dict as well as all other variable values (that are not yet implemented for the results dict)
Its just that it allows for more flexible modeling inside the apps, e.g. take the solved optimization_model modify it solve again or merge with other optimization_model. This is why I think
I would like to keep the whole model and its structure and not only informations.
Take the example of a energy_system which will be calculated 100 times with different demands. Then you do not want to build the model again but rather update the balance constraint with new demand values and solve again (maybe with warmstart for the solver, because results will be similar)

Of course this is still possible without or outside the energy_system class. But as the optimization_model gets the energy_system object (containing simulation object) anyway it would make sense to me to keep the om inside the energy_system (if wanted). I mean this would only be two lines of additional code:

if kwargs.get('keep_model', False): 
   self.om = om 
@simnh

This comment has been minimized.

Copy link
Member

simnh commented Dec 17, 2015

By the way @gnn , can we delete the postprocessing module and the results attributes from the baseclasses then? I think it would be easier to maintain if we had only one place were results are extracted/postprocessed

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Dec 17, 2015

  1. I agree, that it hast to be a possibility to keep the om, but @gnn told me that it is possible within the EnergySystem class without an extra "keep_model" option. If it is not possible I agree to have the "keep_model" option.
  2. I still think that it should be the object to have all relevant results available without using the om.
  3. I agree with removing the postprocessing module. If we do this until the December release there is a very small chance that anybody already uses this module.
@gnn

This comment has been minimized.

Copy link
Member

gnn commented Dec 17, 2015

  1. It's possible and I tried to explain it in the docstring. Basically what you
    would do is this:

    es = EnergySystem()
    om = OptimizationModel(es)
    es.optimize(om) # more verbose/exlicit alternative: es.optimize(om=om)

    and the optimize method will use the given model, so anything that has
    been done to it can be observed by accessing om after optimization.
    If you really want to have it available as an attribute on the energy system
    you are free to do:

    es = EnergySystem()
    es.om = OptimizationModel(es)
    es.optimize(es.om) # more verbose/exlicit alternative: es.optimize(om=es.om)
  2. Me too. ;) But I also think we all agree on this.

  3. Cool. If the results dictionary works as intended so far we can remove
    the postprocessing module and the other attached stuff and merge
    solph-development back into dev. Further development on results
    features and fixes can then happen on their own branch.

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Dec 17, 2015

  1. and 2. The way @gnn described is enough for me. @simonhilpert Do you still want the keep_model option or is it okay for you, too?
  2. Everything works for me, with the new result-dict so far. So, get rid of the postprocessing 🚽 😏
@simnh

This comment has been minimized.

Copy link
Member

simnh commented Dec 17, 2015

1,2,3: No problem anymore!

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Dec 18, 2015

If we merge the features/solph_development branch into the dev branch for the next release it will break the example.

@oemof/oemof-main : May anybody adapt the example?

@simnh

This comment has been minimized.

Copy link
Member

simnh commented Dec 19, 2015

I can adapt the storage and the developing example. (At least I will try... ;) ...

@simnh

This comment has been minimized.

Copy link
Member

simnh commented Dec 19, 2015

Ok, I adapted the storage example as well as the example_app from developing examples. For these examples the postprocessing module of solph is not needed anymore. Can you check if results are still correct @caro-rli in the storage example. (I think they should, but used to be sure...)

I did not touch the two regions_example so far.

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Dec 19, 2015

There is an extra branch for the TwoRegExample. I am responsible for this branch.

@simonhilpert Thank you for fixing the storage example.

@gnn gnn closed this in f91d9bf Dec 21, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment