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

Bileşik Önerileri #19

Closed
wants to merge 10 commits into from

Conversation

mehmetb
Copy link

@mehmetb mehmetb commented Jan 16, 2022

Girilen element(ler)le oluşturulabilecek bileşikler filtrelenir ve en kısa formüle sahip 5 tanesi seçilerek kullanıcıya önerilir. Kullanıcı bu önerilerden birine tıklarsa, bileşik elementleri seçilen bileşiğe uygun şekilde güncellenir ve seçilen bileşik kullanıcıya gösterilir.

out.mp4

#5

Search through all available compounds, populate suggestions, sort
the suggestions by formula length. All these are performed in the
browser since all the compound formulas and elements are already
fetched.

Signed-off-by: Mehmet Baker <[email protected]>
- Don't expect exact element counts if there are less than 5
suggestions.
- Don't suggest the selected compound in the suggestions list.

Signed-off-by: Mehmet Baker <[email protected]>
@mehmetb
Copy link
Author

mehmetb commented Jan 16, 2022

Renklerle ilgili tavsiyeniz varsa açığım :) Yeşil, sayfanın temasının/renk paletinin dışında kaldı sanki biraz.

Kodda performans amaçlı optimizasyon da yapılabilir. 5'ten az öneri bulunduysa tüm formülleri tekrar filtreliyoruz. Halbuki buna gerek olmayabilir. Tüm bileşikleri döndüğümüz kısımda "kesin" olmayan (element sayıları tutmayan) bileşikleri de ayrı bir diziye koyup, öneri sayısı 5'ten az olduğu takdirde o ayrı diziyi öneri dizisiyle birleştirebiliriz.

İkinci bir optimizasyon da öneri listesine yeni eleman eklerken daha seçici davranarak bellek kullanımını optimize etmek olabilir. Yalnızca formülü daha kısaysa öneri listesinden o anki en uzun formül çıkartılarak işlenen formül diziye eklenir. Böylece öneri dizisi hiçbir zaman 5 elemanı geçmez ve daha az bellek kullanılmış olur (bedeli işlem maliyeti).

Ben, biraz da heyecandan, bu yazdıklarımı yapmadan göndermiş bulundum :) İsterseniz yarın akşam bunları da PR'ye ekleyebilirim ya da yeni bir PR açabilirim.

@cagrimertbakirci
Copy link
Member

cagrimertbakirci commented Jan 16, 2022

Çok güzel bir başlangıç, çok teşekkürler! Tam olarak düşündüğümüz şeyi uyguluyor.

Renkleri tasarıma biraz daha uydurmak gerekebilir gibi, evet. :) Tasarım dosyalarını ekledim şimdi, belki renk seçiminde fayda sağlayabilir: Zeplin Tasarım Dosyası: https://zpl.io/bopNyyM

Bir tek bileşik yazım formatında C3H5 yerine C_3H_5 iyi olabilir. Yani atom sayısını subscript yazabiliriz.

Her iki optimizasyon önerisi de çok iyi bence. Bunları yapmak kesinlikle değer katacaktır. Yarına yapabilirseniz harika olurdu!

@mehmetb
Copy link
Author

mehmetb commented Jan 16, 2022

Beğenmenize sevindim :)

Tasarım dosyası linkine erişmeye çalıştığımda "Bu projeye davetli değilsiniz. Lütfen proje üyeleriyle iletişime geçin." diyor.

Tabii, yarın akşama iyileştirmelerle birlikte tamamlamaya çalışacağım👍

@mehmetb
Copy link
Author

mehmetb commented Jan 17, 2022

Tasarım ve performans geliştirmelerini tamamladım:

Compound.Suggestions.Preview.mp4

Genel bir performans iyileştirmesi mümkün; bileşikler kısmı biraz yavaş çalışıyor. "Performans iyileştirmeleri" başlığı açılması iyi olabilir.

Copy link
Collaborator

@cangencler cangencler left a comment

Choose a reason for hiding this comment

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

Katkilarin icin tesekkur ederiz! 🙏

setElementsByFormula methodunu parcalara ayirman mumkun mu? Bu haliyle cok uzun(okunakligi az) ve ilerideki gelistirmeler icin zorluk yaratacak bir yapida oldugunu dusunuyoruz.


for (const { symbol, count } of formulaSymbols) {
const element = this.getElementBySymbol(symbol)
if (element) {
Copy link
Author

Choose a reason for hiding this comment

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

Burada, bileşik formülünde var olan bir sembolün veritabanındaki elementler tablosunda bulunamama ihtimali var mı bilmediğimden, tek tek kontrol ettim. Eğer bu "overkill" ise çok daha basitleştirebiliriz. Mesela:

setElementsByFormula(formula) {
  this.elements = this.parseCompoundFormula(formula).map(({ symbol, count }) => {
    return {
      count,
      ...this.getElementBySymbol(symbol),
    }
  })
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

@eyupcanakman buna bakabilir misin?

Copy link
Author

@mehmetb mehmetb Jan 19, 2022

Choose a reason for hiding this comment

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

Baktım şimdi. Bütün bileşik formüllerini parseCompoundFormula ile parse edip, dönen element sembollerinin hepsini getElementBySymbol ile arattım. Hiç undefined dönmedi. O yüzden ben de bu şekilde basitleştirdim.

@mehmetb mehmetb requested a review from cangencler January 19, 2022 19:00
@cagrimertbakirci cagrimertbakirci added the enhancement New feature or request label Jan 21, 2022
* Group 2: The remaining lower case letter(s) if there are any.
* Group 3: Element count (if specified) (e.g. "2" for O2)
*/
const regex = /([A-Z])([a-z]*)(\d*)/g
Copy link
Collaborator

Choose a reason for hiding this comment

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

Buna test yazabilir misin?

@@ -63,6 +63,11 @@
</swiper>
</div>
</div>
<div v-if="suggestedCompounds.length > 0" class="suggestion-area">
<div v-for="compound in suggestedCompounds" :key="compound.formula" v-fcf class="compound-suggestion" @click="setElementsByFormula(compound.formula)">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Buradaki click eventi icin test yazabilir misin?

@mehmetb mehmetb closed this Sep 6, 2024
@mehmetb mehmetb deleted the mehmetb/compound-suggestion branch September 6, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants