PascalLesage / presamples

Package to write, load, manage and verify numerical arrays, called presamples.
BSD 3-Clause "New" or "Revised" License
14 stars 11 forks source link

Change Indexer __init__ to start at -1 #48

Closed PascalLesage closed 6 years ago

PascalLesage commented 6 years ago

Fixes https://github.com/PascalLesage/brightway2-presamples/issues/47

PascalLesage commented 6 years ago

Need to change tests first. Should get agreement on this first.

cmutel commented 6 years ago

@PascalLesage I know that we discussed this earlier (See email chain "Stupid indexers hurt my brain" from March 5, 2018).

I am no longer familiar enough with the code to understand why we couldn't just take this approach at the very beginning. My hunch is that it is due to compatibility with static LCA calculations. However, there is a clear need for MC to start with the first column, so we will find a way. I will need a bit of time to really understand this, though.

cmutel commented 6 years ago

I think the updated code is a better path. It works with https://bitbucket.org/cmutel/brightway2-calc/commits/a3ca3d60ac11c009776b712202a8cbe99b41cd46.

Here is how the indexers get instantiated and used with my proposed changes:

LCA: .__init__(): Create self.presamples, instance of PackageDataLoader

.load_lci_data(): self.presamples.update_matrices(lci matrices) -> Doesn't update indices because specified matrices

.load_lcia_data():self.presamples.update_matrices(lcia matrices) -> Doesn't update indices because specified matrices

PackagesDataLoader: .__init__(): self.load_data -> creates instances of Indexer (self.sample_indexers)

.update_matrices(): self.update_sample_indices (if for all matrices andadvance_indicesflag)

.update_sample_indices(): next(indexer) for indexer in self.sample_indexers sets self.index to self.count % self.ncols (if sequential) or random

.reset_sequential_indices():

for indexer in self.sample_indexers:
    indexer.reset_sequential_indices()

Indexer: .__init__():

Monte Carlo: .load_data():

LCA.load_lci_data()
if self.presamples:
    self.presamples.reset_sequential_indices()

.__next__(): self.presamples.update_matrices()

coveralls commented 6 years ago

Pull Request Test Coverage Report for Build 97


Changes Missing Coverage Covered Lines Changed/Added Lines %
presamples/loader.py 1 3 33.33%
<!-- Total: 4 6 66.67% -->
Totals Coverage Status
Change from base Build 91: -0.2%
Covered Lines: 813
Relevant Lines: 854

💛 - Coveralls
PascalLesage commented 6 years ago

Much better indeed.