Skip to content

fix: add deepcopy on the way into MemoryCache#444

Merged
danielolsen merged 2 commits intodevelopfrom
daniel/mem_cache_bug
Apr 7, 2021
Merged

fix: add deepcopy on the way into MemoryCache#444
danielolsen merged 2 commits intodevelopfrom
daniel/mem_cache_bug

Conversation

@danielolsen
Copy link
Copy Markdown
Contributor

@danielolsen danielolsen commented Apr 6, 2021

Purpose

Addresses #443.

What is the code doing

Add a deepcopy on the way into the cache, so any modifications to the first returned object don't affect the object in the cache.

Testing

>>> from powersimdata import Grid
>>> grid1 = Grid("Texas")
Reading bus.csv
Reading plant.csv
Reading gencost.csv
Reading branch.csv
Reading dcline.csv
Reading sub.csv
Reading bus2sub.csv
Reading zone.csv
>>> grid1.plant.groupby("type").sum().Pmax
type
coal       14501.59
hydro        555.10
ng         68642.42
nuclear     5138.60
solar       2460.20
wind       19062.63
Name: Pmax, dtype: float64
>>> grid1.plant.Pmax = 0
>>> grid1.plant.groupby("type").sum().Pmax
type
coal       0
hydro      0
ng         0
nuclear    0
solar      0
wind       0
Name: Pmax, dtype: int64
>>> grid2 = Grid("Texas")
>>> grid2.plant.groupby("type").sum().Pmax
type
coal       14501.59
hydro        555.10
ng         68642.42
nuclear     5138.60
solar       2460.20
wind       19062.63
Name: Pmax, dtype: float64

Time estimate

30 seconds to understand, more time possibly to discuss.

@danielolsen danielolsen self-assigned this Apr 6, 2021
Comment on lines +71 to +78
def test_mem_cache_put_version_never_changes():
cache = MemoryCache()
key = cache_key("foo", 4)
obj = {"key1": 42}
cache.put(key, obj)
obj["key2"] = 8675309
assert "key2" not in cache.get(key)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we simplify this test to make it easier to read such as:

def test_mem_cache_put_version_never_changes():
    cache = MemoryCache()
    obj = {"key1": "val1"}
    cache.put("foo", obj)
    obj["key2"] = "val2"
    assert "key2" not in cache.get("foo")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure. I was basically copying and pasting from the test above, but I agree that yours is clearer about what we're trying to do. We could also assert "key1" in cache.get(key).

Copy link
Copy Markdown
Collaborator

@jenhagg jenhagg Apr 6, 2021

Choose a reason for hiding this comment

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

Maybe also assert obj is not cache.get(key)? And possibly assert obj == cache.get(key)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think test_mem_cache_get_returns_copy already catches the first one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@danielolsen danielolsen force-pushed the daniel/mem_cache_bug branch from 05ad0fb to d7af92b Compare April 7, 2021 17:09
Copy link
Copy Markdown
Collaborator

@rouille rouille left a comment

Choose a reason for hiding this comment

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

Thanks

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants