charlesroddie / MathAtom

Structural representations of visual mathematics expressions for use in .Net rendering libraries
MIT License
7 stars 0 forks source link

[WIP] Simplification #8

Closed charlesroddie closed 5 years ago

charlesroddie commented 5 years ago

Glad that you have made a start here @Happypig375 This PR:

I am unsure of:

charlesroddie commented 5 years ago

@Happypig375 is AliasMap related to MathAtom?

Happypig375 commented 5 years ago

It's related to LaTeX-MathAtom two-way conversion.

Happypig375 commented 5 years ago

Replaces System.Memory with char or string where appropriate.

What's the reason for this change?

Removes AliasMap which isn't used and looks like it implements Dictionary functionality.

Dictionary: Key->Value AliasMap: Key1|Key2|Key3|Key4...->Value and Value->Key1

Minor adjustments to MathAtom data.

Struct DUs occupy 4 bytes. Enums occupy 4 bytes. Why lose type safety here?

In SpaceType, how is the ratio between a MathUnit and a point determined? It would be good to simplify this type in some way but it's not clear to me if it is fundamentally a float or a 2D float vector.

MathUnit := FontSizeInPoints / 18

What is the role of type LineStyle = Display = 0 | Text = 1 | Script = 2 | ScriptScript = 3. Removes LineStyle which looked like it partially implements superscripts/subscripts (which is done elsewhere). It also has Display which probabably should not go inside the MathAtom tree as either the whole thing is Display or nothing. What remains is Text so I added that as an MathAtom case.

From CSharpMath. I didn't look into what this does.

Also, the change from unichar to char removes a significant portion of Unicode code points. I'd like to reverse this change.

Brackets as DUs also lose the ability to have user-defined delimiters.

Moreover, the change from the DataTypes folder to MathAtom loses the meaning of sharing types between MathAtom and TextAtom implementation. I'd also like to reverse this change.

Happypig375 commented 5 years ago

Rest looks good.

charlesroddie commented 5 years ago

Replaces System.Memory with char or string where appropriate.

What's the reason for this change?

System.Memory<char> is more complex than char and string which are basic .Net types. It is also used in place of both char and string so is less informative. I think we should keep things as simple as possible and do any performance tuning later.

Removes AliasMap which isn't used and looks like it implements Dictionary functionality.

Dictionary: Key->Value AliasMap: Key1|Key2|Key3|Key4...->Value and Value->Key1

Interesting. My intention was to remove the mutable/immutable scaffolding which isn't necessary, and I thought that was all there was so I removed everything. Can we do this with plain Dictionaries? It will be much simpler without pretending to be immutable. I remember you did a BiDictionary for CSharpMath.

Struct DUs occupy 4 bytes. Enums occupy 4 bytes. Why lose type safety here?

Enums are more primitive objects and don't lose type safety (just disable FS0104 https://github.com/Microsoft/visualfsharp/pull/4522). But this change is not a big deal and I don't mind reverting it.

MathUnit := FontSizeInPoints / 18

Interesting. With the current SpaceType then, if you double the font size, some display sizes will double and others won't (Spacing when SpaceType is in points). Do we want this flexibility, or do we want the guarantee that when you double the font size, the display size doubles? I am somewhat in favour of keeping the guarantee and reducing flexibility.

Also, the change from unichar to char removes a significant portion of Unicode code points. I'd like to reverse this change.

Do we need these code points? Is there any place in the current code where char is inadequate? We may need other unicode code points inside the Text case but that is string data.

Brackets as DUs also lose the ability to have user-defined delimiters.

Do more than 0.01% of LaTeX users define their own delimiters? User-defined delimiters would increase complexity and reduce type safety by more than 0.01% so I am not in favour!

Moreover, the change from the DataTypes folder to MathAtom loses the meaning of sharing types between MathAtom and TextAtom implementation. I'd also like to reverse this change.

OK I can revert but there is not much in here yet and most will get moved elsewhere later.

Happypig375 commented 5 years ago

System.Memory<char> is more complex than char and string which are basic .Net types. It is also used in place of both char and string so is less informative. I think we should keep things as simple as possible and do any performance tuning later.

Sounds good.

Interesting. My intention was to remove the mutable/immutable scaffolding which isn't necessary, and I thought that was all there was so I removed everything. Can we do this with plain Dictionaries? It will be much simpler without pretending to be immutable. I remember you did a BiDictionary for CSharpMath.

Aside from being immutable, it also helps me practise functional programming. :wink:

Enums are more primitive objects and don't lose type safety (just disable FS0104 Microsoft/visualfsharp#4522). But this change is not a big deal and I don't mind reverting it.

Struct DUs also offer more useful methods such as IsX where X is an enum case.

Interesting. With the current SpaceType then, if you double the font size, some display sizes will double and others won't (Spacing when SpaceType is in points). Do we want this flexibility, or do we want the guarantee that when you double the font size, the display size doubles? I am somewhat in favour of keeping the guarantee and reducing flexibility.

Only spacing commands that take units as input can create non-mathunit spaces. Only if the user wants it will it be created.

Do we need these code points? Is there any place in the current code where char is inadequate? We may need other unicode code points inside the Text case but that is string data.

I guess we might have to wait and see. Not reversing for now.

Do more than 0.1% of LaTeX users define their own delimiters?

I don't have the stats but I saw at lease one issue on this in wpf-math: FoRNeVeR/wpf-math#160 Note: lbag/rbag is not currently in CSharpMath as it is non-standard.

OK I can revert but there is not much in here yet and most will get moved elsewhere later.

Ok

Happypig375 commented 5 years ago

** I have the basic completed AliasMap implementation in latest commit, please don't duplicate the work!

Happypig375 commented 5 years ago

I'm going to merge this in a few hours from now.

FoggyFinder commented 5 years ago

Just curious, why there are two separate types for brackets? (LeftBracket & RightBracket)

charlesroddie commented 5 years ago

@FoggyFinder I couldn't decide. You are probably right that one type is better. (type Bracket of | Square etc. with type MathAtom of | Bracket of left: Bracket * inner: MathAtom * right: Bracket).

Happypig375 commented 5 years ago

I also had to revert Bracketed to OpenBracket and CloseBracket because they each represent a character, i.e. brackets need not be balanced. They are treated like ordinary characters.

Happypig375 commented 5 years ago

For brackets that need balancing, Delimited does the job. i.e.\left and \right.