Skip to content
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

Change name? #14

Closed
igouy opened this issue Feb 17, 2017 · 36 comments
Closed

Change name? #14

igouy opened this issue Feb 17, 2017 · 36 comments

Comments

@igouy
Copy link

igouy commented Feb 17, 2017

original-insertion-order-preserving removal would want to be implemented with tombstones, but I've decided this is going to be a separately named hash map. So: fork time.

Just an idle thought -- OrderMap was a good name for something that does guarantee insertion-order-preserved for all operations, so have a new name for this new thing which does not make that guarantee.

Is there a FastMap ? :-)

@bluss
Copy link
Member

bluss commented Feb 17, 2017

Maybe. The main feature is that it has a consistent (iteration) order that does not depend on the keys or the hash function.

@bluss
Copy link
Member

bluss commented Feb 17, 2017

Note that for clarity reasons it did already change name from OrderedMap to OrderMap

@igouy
Copy link
Author

igouy commented Feb 17, 2017

What would you call the "separately named hash map" if this one is still called OrderMap ?

@bluss bluss mentioned this issue Jan 28, 2018
8 tasks
@bluss
Copy link
Member

bluss commented Jan 29, 2018

Renaming the crate seems prudent; the name is the first impression we have, and there we need to deliver the correct expectations about what the data structure does.

NOTE: This is the name for the datastructure in this crate in its current implementation. current OrderMap will change name.

The datastructure is a hash table, stored in a compact indexable range, where it keeps insertion order as long as you don't call .remove().

Name Alternatives:

  1. LinearMap
  2. SequenceMap
  3. IndexMap

I prefer LinearMap because its succinct, vaguely reminds one of the compactly indexable "Like a Vec" ordering semantics.

Thoughts and wo-hoos are welcome. Please leave well thought out alternative names too. Migration will use deprecation with clear message to point to the new crate name, and there will be a migratory crate version that is 100% API compatible with the other.

@bluss
Copy link
Member

bluss commented Jan 29, 2018

Thoughts on new name from @nox @cuviper @igouy @Techcable @pczarn @arthurprs @Binero @garro95 and everyone else!?

@clarfonthey
Copy link

LinearMap implies linear access time, so, no go for that. I'd personally prefer OrderedHashMap or something similar.

@clarfonthey
Copy link

IMO: the name of the type can be succinct but the crate's name should be very precise.

@bluss
Copy link
Member

bluss commented Jan 29, 2018

I'm not going for that meaning of "linear" here. We can describe a Vec as linear too, and it is constant time indexable; I think it can work.

@clarfonthey
Copy link

@bluss that would be fine, but linear-map is already a crate that does linear search. https://crates.io/crates/linear-map

@bluss
Copy link
Member

bluss commented Jan 29, 2018

Aw, that's too bad.

@cuviper
Copy link
Member

cuviper commented Jan 29, 2018

The reason to rename is so an always-ordered crate can assume the name OrderMap?

My next preference is IndexMap, since indexing is such a powerful property here.

@bluss
Copy link
Member

bluss commented Jan 29, 2018

The reason is twofold. Most importantly now that ordermap seems to have delivered the wrong expectations. Secondly yes because I think a full insertion order map is important to Rust, to give the name to it.

@Binero
Copy link
Contributor

Binero commented Jan 29, 2018

For what it's worth, I think @bluss's IndexMap is a pretty descriptive name.

@cuviper
Copy link
Member

cuviper commented Jan 29, 2018

(it wasn't my name, I just picked it from #14 (comment))

@garro95
Copy link

garro95 commented Jan 30, 2018

I like IndexMap too

@arthurprs
Copy link

IndexMap sounds reasonable to me 😄

@Techcable
Copy link
Contributor

Although I am nostalgic for OrderMap, I think it's okay to rename it to either SequenceMap or IndexMap.

@bluss
Copy link
Member

bluss commented Jan 30, 2018

I appreciate the feedback!

Now 😄 I'm sorry to capture you like this (it's entirely voluntary), you who have already commented here or those that happen to read this: co-maintainers are also welcome since a good crate needs shared ownership. Maybe we don't have so much to do? The most important goal is just 1.0, and the name should be the only thing left blocking that..

@cuviper
Copy link
Member

cuviper commented Jan 30, 2018

I'm willing to be a co-maintainer.

@bluss
Copy link
Member

bluss commented Jan 30, 2018

cc @vitiral too, thoughts about the name?

@cuviper Awesome!

Other volunteers are also welcome to help out, of course.

@vitiral
Copy link

vitiral commented Jan 30, 2018

Some other names:

  • IterMap: to show it's performance when iterating
  • DetMap with crate name deterministic_map to show that iterating is deterministic (very important for tests).
  • IndexMap: already mentioned, I really like it. I think it gets to the root of what the crate does (... also note... I didn't actually realize indexing was supported in the public API -- cool!).

I think my vote is IndexMap but I thought I would spitball the others in case someone else has a real idea.

Also, I will be adding this to the ergo_std crate (not yet announced) in the ergo crate ecosystem as I think it should be considered a core type of ergonomic programming. So you chose a good time to rename!

@bluss
Copy link
Member

bluss commented Jan 30, 2018

DeterministicMap is a good point, DetMap is not a very understandable shortening, but deterministic is right in the realm of names we have been shooting for (even SteadyMap).

Note that the current plan is a common release of version 0.4 of ordermap and indexmap that are API-identical (apart from deprecation in ordermap, and the addition of the renamed types in indexmap). Then shortly after that indexmap can be 1.0.

It sounds a bit convoluted but it each step along the plan is simple and actionable. Ideally there should be an indexmap 0.4 that is ready to be rubber stamped as 1.0 soon.

@vitiral
Copy link

vitiral commented Jan 30, 2018

Just so I understand:

  • The types currently in ordermap are moving to indexmap (or some other name) and being renamed accordingly.
  • There will be new types in ordermap that have some different functionality, i.e. always ordered or something.

Is that right?

@bluss
Copy link
Member

bluss commented Jan 30, 2018

(It's basically right) The second point is a later concern, since no implementation exists, it will of course be a breaking version upgrade.

@nox
Copy link

nox commented Jan 30, 2018

I think DetMap is pretty good; even if reader is confused at the Det part for a few seconds, they'll see use deterministic_map::DetMap; somewhere.

@vitiral
Copy link

vitiral commented Jan 30, 2018

DetMap has the issue where it is not very deterministic when you do a remove, so I'm not sure it is actually the best name.

IndexMap kind of has this issue as well -- but then again if you remove a value from a Vec then the indexes of the other values move around, so that is probably fine. They don't move randomly like they do here, but that's probably okay as well...

@clarfonthey
Copy link

@vitrial this can easily be solved by renaming remove to swap_remove. It'd be clear what's happening then.

@Binero
Copy link
Contributor

Binero commented Jan 31, 2018

@clarcharr remove is just a thin wrapper around swap_remove. I think if we're going with IndexMap, it's important to make remove behave like it would on a Vec. This would of course be terribly slow, but if performance is an issue, people can always switch over to swap_remove.

We could of course also remove it completely.

@bluss
Copy link
Member

bluss commented Jan 31, 2018

I don't quite follow why remove() should change behaviour just because the data structure changes name. Maybe a different name is better then.

.remove() is a tricky knot in this case, there is an inconvenient half way solution — no remove method, but an extension trait that adds a .remove() method that wraps .swap_remove(). That makes dropping in ordermap in as a replacement of the standard HashMap one step more complicated.

@Binero
Copy link
Contributor

Binero commented Jan 31, 2018

When I think of an IndexMap I think of a hybrid between a Vec and a HashMap. The functions have the same effect to the indices as a Vec would have, and the same effect to the keys as a HashMap would have.

In a HashMap, the remove function removes the key and the value from the collection. In a Vec it removes the element and shifts all following elements one position forwards.

So, in a IndexMap, it would be intuitive if a remove function would remove the key and value pair, and shift all following elements one position forwards.

The current remove function works like the remove function of a HashMap, but is nothing like the remove function of a Vec. Rather, it acts like the swap_remove function of a Vec.

@vitiral
Copy link

vitiral commented Jan 31, 2018

@Binero while I agree with you that is slightly more expected, it has significant performance penalties. In most cases the fact that removing an element can affect the index of other elements is enough.

I would go the opposite -- add a shift_remove which does what @Binero is suggesting and just make sure docs state the behavior of remove.

In the majority of cases I would expect the performance of remove to be equivalent to HashMap. The specific behavior is normally not important.

@Binero
Copy link
Contributor

Binero commented Jan 31, 2018

In case we want to just focus on the fact the collection works like a HashMap which allows for fast iterating, where the indexing features are merely a side-effect of the way it is implemented (and thus not really part of the core-features), I think @vitiral's IterMap is more fitting.

I'm just not sure if we want such a narrow use-case for the collection while it could easily handle more.

@bluss
Copy link
Member

bluss commented Jan 31, 2018

From discussion in #rust, came the idea that clear docs would be even better than the perfect name.

No name can be so specific that it has the full picture in just the name. I might propose SteadyMap instead. With a unique name we can make the meaning clear in docs.

@arthurprs
Copy link

arthurprs commented Feb 1, 2018

While I get the motivation, IndexMap still feels way better than SteadyMap (which is essentially arbitrary).

@vitiral
Copy link

vitiral commented Feb 1, 2018

@Binero

In case we want to just focus on the fact the collection works like a HashMap which allows for fast iterating, where the indexing features are merely a side-effect

Something that is fast indexing is pretty much by definition fast at iterating, which is why I think IndexMap is probably the better name.

IterMap also has the problem that it sounds like the struct you get when you call IndexMap::iter() 😆

@bluss
Copy link
Member

bluss commented Feb 1, 2018

Thanks a lot for being part of the process, IndexMap it is! And thanks @cuviper that you want to help out!

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

No branches or pull requests

10 participants