search

arfc / d3ploy

A collection of Cyclus manager archetypes for demand driven deployment
BSD 3-Clause "New" or "Revised" License
4 stars 11 forks source link

Decomm #230

Closed FlanFlanagan closed 5 years ago

FlanFlanagan commented 5 years ago

To tackle #11

pep8speaks commented 5 years ago

Hello @FlanFlanagan! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 302:80: E501 line too long (90 > 79 characters) Line 309:80: E501 line too long (97 > 79 characters)

Line 208:58: W291 trailing whitespace Line 233:45: W291 trailing whitespace Line 260:13: E722 do not use bare 'except'

Line 294:80: E501 line too long (97 > 79 characters)

Line 21:80: E501 line too long (96 > 79 characters)

Comment last updated at 2019-10-01 22:48:42 UTC
gwenchee commented 5 years ago

I pulled down your branch, ran python setup.py install and tried running the linear_pow_loss.xml file, and it throws an error of no module found for path libd3ploy.demand_driven_deployment_inst.so. It also fails the pytests. Maybe there is a bug?

gwenchee commented 5 years ago

I pulled the branch and reinstalled d3ploy but it doesn't pass the pytest, maybe there is a bug?

katyhuff commented 5 years ago

@gwenchee can you copy/paste the output of the failed test(s) so that @FlanFlanagan knows which are failing?

FlanFlanagan commented 5 years ago

This is still in my queue, trying to fix the cyclus nosttest segfault right now.

gwenchee commented 5 years ago

So i looked again and the pytest are failing because of d3ploy not properly installing. Seems like a syntax error: image

FlanFlanagan commented 5 years ago

That's odd. I'm not getting that. I didn't even touch line 226 in supply_driven

robfairh commented 5 years ago

I am getting a similar error image

robfairh commented 5 years ago

I've no idea what happened, I could see that you didn't touch those lines, but there is an inconsistency with the use of tabs and spaces in those lines. Once those errors are fixed, it compiles. Did you get those files from the last version of master? @FlanFlanagan

gwenchee commented 5 years ago

Perhaps update the decomm branch with the current arfc/master because it seems to be behind?

gwenchee commented 5 years ago

Current error: 

gwenchee ~/github/d3ploy (decom2) $ python setup.py install
running install
running build
running build_py
package init file 'd3ploy/__init__.py' not found (or not a regular file)
package init file 'd3ploy/__init__.py' not found (or not a regular file)
running install_lib
byte-compiling /home/gwenchee/anaconda3/lib/python3.6/site-packages/d3ploy/demand_driven_deployment_inst.py to demand_driven_deployment_inst.cpython-36.pyc
Sorry: TabError: inconsistent use of tabs and spaces in indentation (demand_driven_deployment_inst.py, line 237)
byte-compiling /home/gwenchee/anaconda3/lib/python3.6/site-packages/d3ploy/supply_driven_deployment_inst.py to supply_driven_deployment_inst.cpython-36.pyc
Sorry: TabError: inconsistent use of tabs and spaces in indentation (supply_driven_deployment_inst.py, line 226)
running install_egg_info
Removing /home/gwenchee/anaconda3/lib/python3.6/site-packages/d3ploy-0.0.1-py3.6.egg-info
Writing /home/gwenchee/anaconda3/lib/python3.6/site-packages/d3ploy-0.0.1-py3.6.egg-info
katyhuff commented 5 years ago

@FlanFlanagan What's the status of this?

katyhuff commented 5 years ago

@FlanFlanagan Can you please move this PR forward?

katyhuff commented 5 years ago

@FlanFlanagan Thanks for moving this forward. There are conflicts in this PR. Can you resolve them?

FlanFlanagan commented 5 years ago

Yep!

FlanFlanagan commented 5 years ago

After working through this a while, I've discovered that Cyclus does not allow the user to modify the lifetime of a facility once built. Or rather there is a function to do it, but it just throws an error.

I'm modifying Cyclus to allow for this. Since we're time crunched I'll just support another branch until we can discuss it at the next cyclus meeting.

katyhuff commented 5 years ago

@FlanFlanagan How are the Cyclus modifications going -- is there a PR to review over there?

FlanFlanagan commented 5 years ago

Still a WIP but as of 5 minutes ago I have a working cyclus branch and a d3ploy branch. Need to add tests, but it appears to be decommissioning.

FlanFlanagan commented 5 years ago

Could you add an integration test for the decommissioning capability?

There is one?

gwenchee commented 5 years ago

Yup, i see the test in the latest commit. Thanks @FlanFlanagan

gwenchee commented 5 years ago

Could you add the decommissioning test into the CI list of tests? @FlanFlanagan

FlanFlanagan commented 5 years ago

Okay, should be good on the d3ploy side. However, cycamore needs to be updated for the test to pass I believe.

It's passing on my machine with the new PR I put into cyacmore.

A side note, and this is very important, with the installed capacity change if you have any facilities in your institution that are not being tracked by D3ploy, it will cause d3ploy to fail.

katyhuff commented 5 years ago

@gwenchee Has @FlanFlanagan handled your comments?

katyhuff commented 5 years ago

@pep8speaks pep8ify

pep8speaks commented 5 years ago

Here you go with the Pull Request ! The fixes are suggested by autopep8.

@katyhuff @FlanFlanagan

gwenchee commented 5 years ago

@FlanFlanagan I don't think you addressed the review comments I made previously.

katyhuff commented 5 years ago

A side note, and this is very important, with the installed capacity change if you have any facilities in your institution that are not being tracked by D3ploy, it will cause d3ploy to fail.

This seems bad.

FlanFlanagan commented 5 years ago

It was bad. Which is why I asked @gwenchee to fix it. Since the installed capacity was part of her work. I didn't see any movement so it is currently fixed in this PR.

It looks like I did address your comments. I'm not sure what I missed but I'm happy to fix it.

FlanFlanagan commented 5 years ago

Ah, sorry. I didn't mark them as resolved! Sorry!

katyhuff commented 5 years ago

ok! @gwenchee @robfairh @kipk49 Can someone take a look and review?

robfairh commented 5 years ago

decommision_test fails on my machine. Do you have the same issue @FlanFlanagan ??

I got this:

=========================================================================== FAILURES ============================================================================
_______________________________________________________________________ test_decommission _______________________________________________________________________

    def test_decommission():
        output_ = 'POWER.txt'
        input_path = os.path.abspath(__file__)
        find = 'd3ploy/'
        indx = input_path.rfind('d3ploy/')
        input_ = input_path.replace(
            input_path[indx + len(find):], 'input/decommission.xml')
        s = subprocess.check_output(['cyclus', input_],
                                    universal_newlines=True, env=ENV)
        with open('POWER.txt') as f:
            lines = f.readlines()
        f.close()
        val = float(lines[-1].split(' ')[5])
>       assert (val < 50.)
E       assert 50.0 < 50.0

tests/integration_tests/decommission_test.py:42: AssertionError

and

________________________________________________________________________ test_backdecom _________________________________________________________________________

    def test_backdecom():
        output_ = 'POWER.txt'
        input_path = os.path.abspath(__file__)
        find = 'd3ploy/'
        indx = input_path.rfind('d3ploy/')
        input_ = input_path.replace(
            input_path[indx + len(find):], 'input/backdecom.xml')
        s = subprocess.check_output(['cyclus', input_],
                                    universal_newlines=True, env=ENV)
        with open('fuel.txt') as f:
            lines = f.readlines()
        f.close()
        val = float(lines[-1].split(' ')[5])
>       assert (val < 50.)
E       assert 50.0 < 50.0

tests/integration_tests/decommission_test.py:58: AssertionError
robfairh commented 5 years ago

I also tried running input/decommission.xml and I got this:

roberto ~/github/d3ploy/input (decomm) $ cyclus decommission.xml 
              :                                                               
          .CL:CC CC             _Q     _Q  _Q_Q    _Q    _Q              _Q   
        CC;CCCCCCCC:C;         /_\)   /_\)/_/\\)  /_\)  /_\)            /_\)  
        CCCCCCCCCCCCCl       __O|/O___O|/O_OO|/O__O|/O__O|/O____________O|/O__
     CCCCCCf     iCCCLCC     /////////////////////////////////////////////////
     iCCCt  ;;;;;.  CCCC                                                      
    CCCC  ;;;;;;;;;. CClL.                          c                         
   CCCC ,;;       ;;: CCCC  ;                   : CCCCi                       
    CCC ;;         ;;  CC   ;;:                CCC`   `C;                     
  lCCC ;;              CCCC  ;;;:             :CC .;;. C;   ;    :   ;  :;;   
  CCCC ;.              CCCC    ;;;,           CC ;    ; Ci  ;    :   ;  :  ;  
   iCC :;               CC       ;;;,        ;C ;       CC  ;    :   ; .      
  CCCi ;;               CCC        ;;;.      .C ;       tf  ;    :   ;  ;.    
  CCC  ;;               CCC          ;;;;;;; fC :       lC  ;    :   ;    ;:  
   iCf ;;               CC         :;;:      tC ;       CC  ;    :   ;     ;  
  fCCC :;              LCCf      ;;;:         LC :.  ,: C   ;    ;   ; ;   ;  
  CCCC  ;;             CCCC    ;;;:           CCi `;;` CC.  ;;;; :;.;.  ; ,;  
    CCl ;;             CC    ;;;;              CCC    CCL                     
   tCCC  ;;        ;; CCCL  ;;;                  tCCCCC.                      
    CCCC  ;;     :;; CCCCf  ;                     ,L                          
     lCCC   ;;;;;;  CCCL                                                      
     CCCCCC  :;;  fCCCCC                                                      
      . CCCC     CCCC .                                                       
       .CCCCCCCCCCCCCi                                                        
          iCCCCCLCf                                                           
           .  C. ,                                                            
              :                                                               
/home/roberto/anaconda3/lib/python3.6/site-packages/sklearn/externals/six.py:31: DeprecationWarning: The module is deprecated in version 0.21 and will be removed in version 0.23 since we've dropped support for Python 2.7. Please rely on the official version of six (https://pypi.org/project/six/).
  "(https://pypi.org/project/six/).", DeprecationWarning)
/home/roberto/anaconda3/lib/python3.6/site-packages/sklearn/externals/joblib/__init__.py:15: DeprecationWarning: sklearn.externals.joblib is deprecated in 0.21 and will be removed in 0.23. Please import this functionality directly from joblib, which can be installed with: pip install joblib. If this warning is raised when loading pickled models, you may need to re-serialize those models with scikit-learn 0.21+.
  warnings.warn(msg, category=DeprecationWarning)
 ERROR(core  ):SQL error [INSERT INTO AgentState_d3ploy_demand_driven_deployment_inst_DemandDrivenDeploymentInstInfo VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?);]: table AgentState_d3ploy_demand_driven_deployment_inst_DemandDrivenDeploymentInstInfo has 20 columns but 22 values were supplied
katyhuff commented 5 years ago

@FlanFlanagan @robfairh it seems like this new decommission_test.py isn't being included in the CI. Please add this to CI in the "full integration test" part and see if it passes on CI.

https://circleci.com/gh/arfc/d3ploy/538?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Also -- rather than running specific test_*.py files, why not run py.test in the directory?

robfairh commented 5 years ago

It passes the CI in #292. I haven't updated my Cycamore after Robert's PR, I'll do it and see if the tests pass on my local.

robfairh commented 5 years ago

I updated Cyclus and Cycamore and now the tests for decommisioning are passing.

FlanFlanagan commented 5 years ago

@katyhuff shouldn't this go with the other functionality tests? It is being run there, I just looked at those tests and it is listed.

I think the failures that @robfairh were seeing were because he did not have an updated cyclus/cycamore. If the structure of the testing is inaccurate we can fix that but I think that should be a separate PR as the testing update here matches previous style.

katyhuff commented 5 years ago

Ah. Yes, I see. I expected it in the "integration tests" so I looked there and didn't see it, but you're right, it's in "functionality tests". Thanks @FlanFlanagan

robfairh commented 5 years ago

I ran input/decommision.xml and I got this plot for power: decom2-power

I also added the deployment of 'source' facilities and 'Sinks' in this input file decommission3.txt and these are the plots for them: test1 test2

The decommission capability seems to be working. I am ready to merge this PR when @FlanFlanagan merges the PR that solves the PEP8 issues https://github.com/FlanFlanagan/d3ploy/pull/2.

FlanFlanagan commented 5 years ago

Pep 8 taken care of! @robfairh

robfairh commented 5 years ago

Cool! Would it be possible to add a new variable to activate or deactivate the decommissioning capability? Like record: when it is 0, it is not used, and when it is 1, it is used. Sorry @FlanFlanagan this is the last thing. Another way to do it, simpler, would be to set the default value of 'os_time' to a large value (like 1000).

FlanFlanagan commented 5 years ago

Yeah, is there a reason you want it turned off?

robfairh commented 5 years ago

Yeah, so it is backward compatible with the all the inputs we already have.

FlanFlanagan commented 5 years ago

Okay, I adjusted the default os_time